Base class properties not included in indicators factory methods

[wbennettjr]

The properties for base classes are not included in the generated factory methods for indicators. Example:

 public class TrendIndicatorBase : Indicator
{
    private string _test = "test";
  
     [NinjaScriptProperty]
    [Display(Name = "Test", Description = "Testing.", GroupName = "Parameters", Order = 0)]
    public string Test
    {
        get { return _id; }
        set { _test = value; }
    }
    
    public void override OnBarUpdate()
    {
    }
}
 public class OptimisticTrend : TrendIndicatorBase
{
}
 public class AggressiveTrend : TrendIndicatorBase
{
}

TrendIndicatorBase would have the Test property included but the others won't.

[NinjaTrader_Jesse]

Hello,

Thank you for providing the sample on this.

I am currently reviewing the sample to determine what is happening here. Once I have some more information on this I will reply back .

I look forward to being of further assistance.

[NinjaTrader_Jesse]

I wanted to follow up on this item.

It looks like this is not currently matching NT7 so I have submitted this item to development for further review.

Thank you for providing the sample on this, that is very helpful.

I look forward to being of further assistance.

[wbennettjr]

Thanks Jesse! No problem.

[NinjaTrader_Jesse]

Hello,

It was requested from development to get a greater sample that demonstrates a usage of the classes you have defined.

I created a sample where I had Instantiated a class from an indicator similar to yours but that was an assumption.

Can you instead provide an example that demonstrates exactly how these classes would be instantiated, used or called and specifically what property you are trying to inherit and use?

I look forward to being of further assistance.

[wbennettjr]

Sure Jesse. No problem.

Here's one scenario:

I want to use an indicator in a strategy and also want to show the indicator on two charts (one with the strategy and one without). The strategy needs to change the parameters of the indicator but it should not affect the instances that are shown on the charts.

So, if I have SimpleStrategy and SimpleEntryIndicator defined as follows:

 public class SimpleEntryIndicator : Indicator
 {
     [NinjaScriptProperty]
     [Display(Name = "Strong Signals Only")]
     public bool StrongSignalsOnly { get; set; }
 }
  
 public class SimpleStrategy : Strategy
 {
     private SimpleEntryIndicator indicator;
  
     protected override void OnStateChanged()
     {
         if (State == State.Configure)
             indicator = SimpleEntryIndicator(true);
     }
  
     protected override void OnBarUpdate()
     {
         if (someCondition)
             indicator.StrongSignalsOnly = true;
         else
             indicator.StrongSignalsOnly = false;
         ...
     }
 }

This does not work properly because the indicator is cached by the input and the StrongSignalsOnly property. This means the other references on the charts will be changed when the strategy updates the property. If I create another chart, it may or may not be in sync with the other charts depending on what the property value is when the chart is created. I may also not want to make the property part of the factory function because I may not want to specify the property each time creating the indicator (The indicator is getting cached by the property so it shouldn't be changed anyway - just for illustration). Currently, there is no way to add a property that has a default value in the generated methods and I created a thread for that (http://ninjatrader.com/support/forum...ad.php?t=78783).

If I didn't make the property a NinjaScriptProperty (which would be the correct way for this scenario), it gets even worse because now the indicator is cached only by Input so everything will always share the same instance.

One way to fix this is the following:

  public partial class Indicator
  {
      private string _id = "global";
  
      [NinjaScriptProperty]
     [Display(Name = "Indicator ID")]
     public bool Id { get { return _id; } set { _id = value; } }
 }

  public class SimpleEntryIndicator : Indicator
 {
     [NinjaScriptProperty]
     [Display(Name = "Strong Signals Only")]
     public bool StrongSignalsOnly { get; set; }
 }
  
 public class SimpleStrategy : Strategy
 {
     private SimpleEntryIndicator indicator;
  
     protected override void OnStateChanged()
     {
         if (State == State.Configure)
             indicator = SimpleEntryIndicator("simplestrategy", true);
     }
  
     protected override void OnBarUpdate()
     {
         if (someCondition)
             indicator.StrongSignalsOnly = true;
         else
             indicator.StrongSignalsOnly = false;
         ...
     }
 }

This provides a consistent way to get specific instances of *any* indicator. Now, the strategy has its own instance of the indicator (which can be shared with other strategies, indicators or charts if needed) and I'm free to add other instances on other charts. So on a chart I could have the strategy only looking for strong signals but also show the weak signals with an instance of the indicator on the chart. I can now also choose not to add the [NinjaScriptProperty] attribute to the StrongSignalsOnly property because the indicator will be cached by the Input and Id properties.

There still remains a problem with this though. If the feature is implemented as is, I would be forced to provide the Id as a parameter to the factory methods each time. Ideally, what I would want is:

  public partial class Indicator
  {
      private string _id = "global";
  
      [NinjaScriptProperty(Default = "global")]
     [Display(Name = "Indicator ID")]
     public bool Id { get { return _id; } set { _id = value; } }
 }

  public class SimpleEntryIndicator : Indicator
 {
     [NinjaScriptProperty(Default = false)]
     [Display(Name = "Strong Signals Only")]
     public bool StrongSignalsOnly { get; set; }
  
      #region NinjaTrader generated
     public SimpleEntryIndicator SimpleEntryIndicator(string id = "global", bool strongSignalsOnly = false)
     {
         return SimpleEntryIndicator(Input, id, strongSignalsOnly)
     }
  
     public SimpleEntryIndicator SimpleEntryIndicator(ISeries<double> input, string id = "global", bool strongSignalsOnly = false)
     {
         // Caching logic with indicator cached by Input, Id, StrongSignalOnly
         ...
         return cachedVersion ?? AddToCache(SimpleEntryIndicator(Input, id, strongSignalsOnly));
     }
     #endregion
 }
  
 public class SimpleStrategy : Strategy
 {
     private SimpleEntryIndicator indicator;
  
     protected override void OnStateChanged()
     {
         if (State == State.Configure)
             indicator = SimpleEntryIndicator("simplestrategy", true);
     }
  
     protected override void OnBarUpdate()
     {
         if (someCondition)
             indicator.StrongSignalsOnly = true;
         else
             indicator.StrongSignalsOnly = false;
         ...
     }
 }

Now it's much more flexible on how the indicator is created and shared (which was possible in NT7 - a bunch of my NT7 indicators are broken because of this). I could manually create the indicator outside of the cache and set the properties but then it complicates the scenario where you want one indicator/strategy to control the environment. e.g. You have one indicator that determines when we should be looking for strong signals only. It creates an instance with SimpleEntryIndicator("trading"); and SimpleStrategy and ComplexStrategy do the same. Now both strategies will switch to looking for strong signals only at the same time while the charts are free to show both strong and weak signals.

Personally, I can understand adding and augmenting features but I don't think removing features from version to version is such a good idea.

[NinjaTrader_Jesse]

Hello,

Thank you for the detailed reply, unfortunately I am unsure on what the end goal is based on what is provided. Could you provide more details on what the goal is regarding what you are doing in this example?

I would need a more simplified clarification for development as I myself do not really understand what you are trying to accomplish and need further information to relay to development.

Also if you could provide a working script from NT7 that can compile and that shows what you are trying to accomplish in a simplified form that may be helpful.

I look forward to being of further assistance.

[wbennettjr]

Hi Jesse,

I'm not sure how to help you - I'm really confused. I started with a simple example which you verified works in NT7 but not NT8. That is the simple working script that demonstrates the issue. You asked for a more detailed explanation, which I provided. Now you are asking for a simple example!

FWIW, I'll summarize what I've already posted.

The end goal:

If you use NT8 to compile the script from the original post, you will notice that the generated factory methods in the OptimisticTrend and AggressiveTrend indicators do not include the Test property from their base class, TrendIndicatorBase. If you do the same in NT7, they are. That is the issue.

Additionally, it would be good to have default/optional parameters as was the case with NT7 (at least the optional ones were). This is illustrated in the last code snippet with the Indicator and SimpleEntryIndicator indicator classes. Again, compile the two indicators from the script in NT8 (Minus the example generated code I provided. You'll need to add an OnBarUpdate override for it to compile) and look at the generated factory methods in SimpleEntryIndicator. The point is they should look more like the example generated code - with default parameter values.

I hope this clarifies. I don't know how else to say it otherwise.

[NinjaTrader_Brett]

Thanks for the clarification. We were on the wrong track since we were working on the actual properties be missing from the derived type.

This is about support for indicators which are derived from a class which is derived from Indicator. To access an indicator from another indicators,strategy, Market Analyzer column NinjaTrader generated code at the bottom of an indicator allows this to happen. Its generated each time you compile and is what enabled that functionality.

With NinjaTrader 7 we did this based on the folder your .CS files were located. Based on the expanded NS support we are having in NT8 we decided to move to a regex based approach where we scan to see if a class is derived from Indicator and if so generated wrapper code at the bottom of the file.

This is an expected difference between NT7 and NT8 which means a wrapper wont be generated for custom IndicatorBase classes. I will add to the list the demand to go back to a folder like approach like NT7 to support this use case.

Thanks for taking the time to work through that with us.

[wbennettjr]

Thanks for the response Brett!

To be clear, wrapper code is getting generated for each class in the hierarchy (except any placed in the AddOns folder). It's just the leaf classes don't include the properties (in the generated code) of any classes above them (not even properties in Indicator). I believe this is what you are saying - just wanted to make sure we're on the same page.

Also, the properties and other regular properties (that don't have the [NinjaScriptProperty] attribute) are getting picked up correctly and shown in the properties grid - so this is just a code generation issue. Again, I think your information about using regex for the generation explains the reason for this.

[NinjaTrader_Brett]

No problem and thanks for writing back to clarify. We are almost on the same page but don't think we are there yet. In my test I have no wrapper code generated as I expect for derived types from a derived indicator. Are you using beta 5?

Example:

Test A: http://screencast.com/t/pynwnQprTNvN
Has the collapsed "NinjaScript generated code. Neither change nor remove. at the bottom and is derived from Indicators. Works as expected.

Test B: http://screencast.com/t/wzDezUsNxVx
Does not have the generated code at the bottom and is derived from "TestA" indicator class which is expected.

[wbennettjr]

Yes, I am using Beta 5.

It seems the issue is more insidious than first glance. I created two indicators like you did in the screenshots and got the same results you got. This is REALLY weird because I've been doing this without issue since I started using the beta. I have only one indicator that derives directly from Indicator. All other indicators are derived from a child class of Indicator and I've had no problems with it generating code.

I've been trying to figure out what the difference is but am having no luck so far. If I change the base class of TestB to my base class (that is derived from Indicator), the code IS generated. If I change it back to deriving from TestA, the code is NOT generated!

I develop in Visual Studio and when I make a change to TestA and save, NT compiles (chime is heard and the code updated in the NT editor). When I make a change to TestB (when it derives from TestA) from Visual Studio and save, NT does not compile and the code is not updated in the NT editor!

This is all very weird. I'm still trying to find out what conditions will cause it to generate the code for grandchildren.

[wbennettjr]

Aha! I figured it out! Your developers have made a big boo boo!

After re-reading your post about how the inheritance is determined, I spotted a clue (regex) and came up with a theory that proved to be true. Apply any of the following changes to have the code generated:

 /* I can't believe public class TestB is not butter! Broken code: indicator of anything? */
 public class TestB : TestA // Indicator
 ...

 public class TestB : TestA // Indicator
 ...

  public class TestAIndicator : Indicator
  ...
 
 public class TestB : TestAIndicator
 ...

One interesting side effect is that comments can now cause indicators to break! If you make any of the following changes, the indicator will fail to compile!

 /* This used to be "public class TestC : MyIndicator" but there were issues. */
 public class TestB : TestAIndicator
 ...
  
 or
  
 /* Changed from another public class in version 1: TestD.  An indicator of differences in version 2. */
public class TestB : TestAIndicator
...
   
 or
  
[Notes("There is another public class for this: it's TestZIndicator.")]
public class TestB : TestAIndicator
...

Based on my tests, it seems the regex used is something similar to the following snippet. The regex is clearly wrong and I think the "correct" regex would probably need to be used in conjunction with a parser in order to exclude comments.

 Regex.IsMatch(code, @"^\s*(?!//).*public\s+class\s+(?<name>\S+).*Indicator", RegexOptions.IgnoreCase);

<rant>
This is a terrible use of regular expressions. The resulting code will be very brittle. A much better approach would be to compile the code and use reflection to determine hierarchy, discover properties, etc... I think this and some of the other design choices are really going to come back to bite when it's time to develop NinjaTrader IX (I suspect they are probably causing a lot of bug reports in the betas and probably caused a lot of issues during development as well). The two main offenders are introducing static classes and going from interfaces to concrete classes. This is DEFINITELY the wrong direction, IMHO. As per the SOLID principles of software development, one should code against abstractions not "concretions". These will certainly bite down the road.
</rant>

<ShamelessPitch>
This particular scenario would be, IMHO, best solved by upgrading to .Net 4.6 with C# 6.0. This is one of the prime use cases for the Roslyn API.
</ShamelessPitch>

In case my shameless pitch isn't enough, could you pretty please upgrade to .Net 4.6? I'll throw in as many pretty pleases as you guys want

P.S.

If you're hiring developers, I'm currently available. I primarily do C# and WPF but have also done C++, Ruby, Delphi and others.

[NinjaTrader_Brett]

Thanks for isolating that out, would make sense. I will forward that to the developer.

As to the <Rant> reflection would not fly or would be to unwieldy during the coding process for clients. Since we ultimately needed to add code to the file since the purpose of the code at the bottom of the file is to allow other users to use c# programming and reference your indicator during their development of their own indicators/strategy. Reflection only works in run-time and not prior to compile time where a user will be programming. Plus a user will want to enjoy all the benefits of intellisense etc. Point is reflection is for runtime use (Which we do in fact utilize reflection heavily for the exact purposes you point out) and the code at the bottom of the file is for a compile time use.

As to the <ShamelessPitch> you are correct the roslyn compiler could be used for that purpose and something we could look into. However its something that implementing the Roslyn compiler could be tricky, its much more complex of a task then what we are doing now that currently works for the majority of our user base. We did look into .net 4.6 however we will not be upgrading in the short term since most PC's don't have .net 4.6 installed yet and its just too early to adopt it (Last month there was even critical .net 4.6 bugs that would cause random application crashed being hotfixed). Its just not ready yet.

Finally we have two open developer positions, please see them here:

https://ninjatrader.bamboohr.com/jobs/view.php?id=14

https://ninjatrader.bamboohr.com/jobs/view.php?id=13

[wbennettjr]

Thanks Brett!

All questions have been answered so I think this thread is complete. Without the intention of stretching it out, I just want to reply to your comment about reflection.

Using reflection would not affect the user's ability to have intellisense or any other feature. The only thing it would add is an extra compile cycle. The current process is that the user makes a code change, compiles the code and then the code generation happens (note: intellisense would not pick up any factory method signature changes since generation does not happen until the code is compiled). That would not change. The difference is you would compile the code, use reflection to do the code generation and then compile again. That's what Roslyn would do as well - you have to compile to get semantic analysis. From the user's perspective, nothing changed. The benefit of Roslyn is you could do the generation without waiting for the user to do an explicit compile! That *would* enhance intellisense as the generation could happen in real time as the user is typing!

Thanks a lot for your help! I really appreciate your time.