Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Multiple Culture Support, Extended two indicators #2

Merged
merged 1 commit into from
Jun 26, 2017

Conversation

fernaramburu
Copy link
Contributor

Hi, first of all, great job on this amazing library!
I did some extensions and some little improvements that might be nice to have directly on your code and not in my branch, so others get this.

Add Culture to CSV Importer to avoid failing when running on machines with different cultures.

Extend Indicators ClosePricePercentageChange and ClosePriceChange to support indicators for different number of days
Add both indicators extensions to the CandlesExtension class. I added another method for each option so users of the other already existent option doesn't need to change parameters.
Added tests for everything

If you want to discuss more you can find me through skype @fernandoaramburu.
Hope this help.

… with different cultures.

Extend Indicators ClosePricePercentageChange and ClosePriceChange to support indicators for different number of days
Add both indicators extensions to the CandlesExtension class. I added another method for each option so users of the other already existent option doesn't need to change parameters.
Added tests for everything
@karlwancl
Copy link
Owner

@fernaramburu . Thanks for the praise & the PR :) It's always nice to see someone find it useful & willing to contribute to it, much appreciated! Future users will thank you on this :)

It's great to share ideas & have discussions on features & codings but I do not use Skype. It would be nice if we could just talk here for new ideas or features :)

@karlwancl karlwancl merged commit 3b8ff3b into karlwancl:master Jun 26, 2017
@fernaramburu
Copy link
Contributor Author

@lppkarl Thanks for the words and the merge. I'm getting old by suggesting Skype? :) I have some ideas. You actually "kill" with the latest change to version 2.0 on removing dates from everywhere but I will figure out something there.

By the way, I also implemented a small library to download prices from Google (you can find it through my user in github or at nuget with name Nuba.Finance.Google). We can add it to the importer library, altough is not a crucial feature at all.

One of my purpose with this library is to get prices, calculate indicators for long periods and then save it to CSV or azure databases. I will think on a simple way to do this and I will suggest something for Trady library.

Other ideas I have for my product is to use what you implemented on the portfolio backtesting but not for backtesting as is but as a way to collect information on custom buy and/or sold signals just to try to analyze if the signals generated where efficient or not.

If you want to discuss any of the features you have in mind, please feel free to share them with me and maybe we can work together on them.

Thanks

Fer

@karlwancl
Copy link
Owner

karlwancl commented Jun 27, 2017

@fernaramburu , Skype is a great product, it's just my own preference not to use it, and I want to keep all our conversation in records here so that latecomers and community users can see what's going on with this project. It helps bringing up interested users, and may get more innovative ideas from them :)

There is reason behind the removal of ITick & its related code. Indicator calculation is actually independent of the value of "DateTime" but dependent of the order of "DateTime". I would like to abstract away "DateTime" from the input object & output object, and only use array index as the order for the calculation. It makes implementation of ComputeByIndexImpl clearer by returning only the calculated value rather than including the "DateTime" in the calculation implementation.

But what you've mentioned is also important for an ordinary user, it's really inconvenient for them to map back the DateTime to the result. I've created a branch (1142896) for showing the concept of implementing back datetime to the calculation. The main idea of it is to create a wrapper to the original indicator class and returns the result with "DateTime" property. I would like to know what you think on the implementation. I am hesitating if it is better to extend the original AnalyzableBase with a new method "ComputeToTick" or follows what I have implemented in the branch. It seems the latter is more relevant because it guarantees there is DateTime from the input object, there must be DateTime value to output, but the former seems more user-friendly as it provides one class for all uses.

For the google finance lib, I can help integrate it to the library, it should be easy for the integration, and I can also take a look on your library, too :)

By what you tell, I am going to launch a project for it, do you mind I add you to the collaborator list for the project? We can work together on your ideas :)

@fernaramburu
Copy link
Contributor Author

@lppkarl Nice to see the two issues you created. Now that you pointed out that written conversation left notes of it, I agree about not doing a call (skype or whatever).

I understand your point on removing the logic of DateTime of actually the entire Candle object. Anyway I don't fully agree on the sentence "Indicator calculation is actually independent of the value of DateTime". Not to make a discussion about that but "my custom indicators" that i'm trying to look for (plus all the one you already coded) are almost all related to DateTime. Like what you might be asking? Like a ClosePricePercentageChange on this year, this quarter, this month, this week or others like the location between 0 and 1 where 0 means the lowest value of the last 52 weeks and 1 the highest value in the same period. I have others but I guess you can understand my point on this.

Reading the newspaper of the following day (that's kind of a saying in Argentina, maybe all over the world too) I would have implemented in a different way keeping the candles like a List where T is generic and then passing a function (T -> decimal) that provides the decimal value I like to compute. That way, I can add logic to the ComputeByIndexImpl using the Candle object or the instance of the selected T element while having the chance to access the decimal I want through the function. Something similar what you did but a bit different in terms of if someone wants to write and indicator that keeps track of the differences between the Open and Close in the candle it can be done.

If I'm not being clear then let me know and I'll try to give you some code example. Tomorrow I'll be probably out but I'll answer the day after or ASAP.

About the importer from Nuba.Finance.Google, and external project is perfect. I would even say that we can do different small projects for each different importer and then name them like Trady.Importer.Quandl, Trady.Importer.Google, Trady.Importer.Yahoo, Trady.Importer.Csv and so on.

Please , be my guest on adding me as a contributor. It's a pleasure and who knows if there is something huge starting here :)

Thanks

Fer

@karlwancl
Copy link
Owner

karlwancl commented Jun 28, 2017

@fernaramburu , I get your point on the importance of DateTime, so I am adding DateTime back to the library by creating a wrapper around the original indicator and the draft is in the commit 1142896. You can take a look at the wrapped indicator here, and the generic wrapper here.

The general idea is to mimic the interface of what i did in v1. That is we intake a IList<Candle> instance & the required parameter, compute it and returns the indicator result (with DateTime). It should be the same with the interface of v1, migration should be less hard.

Or if you just want to create your own new indicator which the input is of IList<Candle> & returns some decimal values with DateTime, you can also directly inherit your custom indicator from AnalyzableBase<Candle, AnalyzableTick<decimal?>>.

AnalyzableBase class get its input type & output type from its generic arguments, i.e. TInput & TOutput, you can choose your input & output type whatever you like. The AnalyzableBase class simply extends the old one from input of IList<Candle> to IList<something>, what you can do in v1 should also work in v2, the only thing you have to do is to set TInput = Candle in the generic argument.

For the importer, it's architecturally well to divide different importers into different pieces, as a make-all-things-atomic developer, I can't agree more on this. But I wonder if it would be too small for each importer to have its own package. I mean the importer itself only implements a single method, and it's just an adapter to other stock data importing library, e.g. The Trady.Importer.Quandl is referencing Quandl.NET (also 1 of my project). It would have some overhead for users to install some/all of them which may not worth the division.

However, we can do this by seperating them into 2 sets, one is Trady.Importer.dll for all bundled, the other is Trady.Importer.xxx.dll for different importers. Is it OK for you for this approach?

By the way, we could continue our discussion in the issue threads, it's better to discuss the related ideas in the related threads. You may also add some ideas by creating a new issue thread, we can discuss more on that :)

Edit: I've just send you an invitation on collaboration, please accept it, thanks :)

@fernaramburu
Copy link
Contributor Author

@lppkarl hey, I just did a commit with tested sample code and put some comments on the DateTime issue. I'm not sure if you really understand my need on indicator calculators so I provide with test code to give you real life scenarios.

On the other issue I will put comments in there so current thread would be ending now :) .

Thanks!

Repository owner locked and limited conversation to collaborators Jul 5, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants