Closed TomTorger closed 2 years ago
Made most corrections i believe :-)
Will have to take a closer look at the .json flows.
Still some changes to perform:
-As you suggested, I think I will add the setpoint as an input. It removes the need for a couple of function nodes.
-Some node documentation left.
-Converge on the statistics payload... The second output yields a lot of statistics, however, it is not very well structured. I should hava a look at it.
Still some changes to perform: -As you suggested, I think I will add the setpoint as an input. It removes the need for a couple of function nodes. -Some node documentation left. -Converge on the statistics payload... The second output yields a lot of statistics, however, it is not very well structured. I should hava a look at it.
Also, please use the same kind of casing in your output as is used in other nodes.
Updated. There are quite a few changes... I've added node tests, updated the documentation, added setpoint support, and boosted temperature functionality. I've also switched to camelcase in the code, however, there are some remaining snake_case in the tests. Will try to fix them in the next round.
This is really getting good!
I collect all my comments here now.
Code:
Very good improvements to the code!
I just found a few details:
You should use the roundPrice() function from utils on prices before they are sent to the output. That will round them nicely to 4 decimals. Examples should be updated with 4 decimals. That looks better, and some of the roundings are actually wrong.
In the strategy-heat-capacitor.test.js
file you are using the Array .at() function. This requires node version 16.6 or later.
Many users are on earlier versions of node, so I recommend not using it yet. All other code runs on node 14, and I sometimes get requests from people using even earlier versions.
Output 3:
The variables that are config should be put in a config object, same as on other nodes.
On the trades, you have sellPrice and buyPrice. What if a sell/buy period lasts more than an hour, will you not get multiple prices then? Or do I misunderstand?
Other nodes have "time" and "version" in the output. These can be useful for debugging. I suggest you do the same. Time is the time the plan is running.
Could you have a schedule too in the output, showing planned time and value that will be sent to output 1? That would make it easier for users to understand what is happening, and some may use it in dashboard etc.
I also think it would be useful to have an hours-array in the output showing start and price. It is very helpful when people are asking for support. Also the "source" attribute should be there to show where prices came from.
Other:
If you use a spellchecker in vscode, you will see some names getting a warning, like Datetime and Dictlist. I like having as few as possible warnings in the editor.
You have not run prettier on all code. I suggest running it on save.
Doc:
Very good updates to the doc too :-)
But still some things that can be improved:
Please show json example for output 2. There are still 3 outputs, yes?
Could you give a brief explanation of the trades and temperatures arrays? Just so people get what it is.
I think buyIndex and sellIndex also need explanation.
If you run npm run docs:dev
you can serve the doc locally and see exactly how it is rendered. I use Vuepress 2 for the docs. There are some very useful custom containers that you could use: https://v2.vuepress.vuejs.org/reference/default-theme/markdown.html#custom-containers
For example, you could use the details custom container instead of the details html tag to get a better styling.
In the examples you are using the blockquote notation. These are not quotes, so maybe some other means could be better. Some places you could just use normal bullet points, other places maybe plain text is better. Run locally to see how it renders. Buy the way, I did not know about the blockquote in Markdown, so thank you for teaching me :-)
Please add links to the examples in the examples/README.md
file
Not sure I got everything here. I managed to write all the feedback for then loosing it by a mistake, so I had to write everything again. I may have forgotten something...
Summary:
You should use the roundPrice() function from utils on prices before they are sent to the output. That will round them nicely to 4 decimals. Examples should be updated with 4 decimals. That looks better, and some of the roundings are actually wrong.`
Done
In the strategy-heat-capacitor.test.js file you are using the Array .at() function. This requires node version 16.6 or later. Many users are on earlier versions of node, so I recommend not using it yet. All other code runs on node 14, and I sometimes get requests from people using even earlier versions.
Fixed
Output 3: The variables that are config should be put in a config object, same as on other nodes. Done On the trades, you have sellPrice and buyPrice. What if a sell/buy period lasts more than an hour, will you not get multiple prices then? Or do I misunderstand?
The sellPrice/buyPrice indicates the average cost pr. kWh. Given that the prices at 03:00 to 04:00 and 04:00 to 05:00 are 1 and 2 NOK/kWh, respectively, a 60 min heating period starting at 03:30 and ending at 04:30, will attain an average price of 1,50 NOK/kWh. The algorithm can also support non-flat procurement patterns, but for now, each minute is given even weight in the sellPrice/buyPrice.
Other nodes have "time" and "version" in the output. These can be useful for debugging. I suggest you do the same. Time is the time the plan is running.
Done
Could you have a schedule too in the output, showing planned time and value that will be sent to output 1? That would make it easier for users to understand what is happening, and some may use it in dashboard etc.
I could do that, but the output would be huge... :-) I store potentially 72 hours of priceData, generating potentially 4320 values. The temperature array at the end is an indexed array, where each value represents the temperature adjustment at "timeStart" + index * 1 minute
I also think it would be useful to have an hours-array in the output showing start and price. It is very helpful when people are asking for support. Also the "source" attribute should be there to show where prices came from.
Done
Other: If you use a spellchecker in vscode, you will see some names getting a warning, like Datetime and Dictlist. I like having as few as possible warnings in the editor.
Done
You have not run prettier on all code. I suggest running it on save.
Done, might have missed some spots...
Doc: Very good updates to the doc too :-) But still some things that can be improved: Please show json example for output 2. There are still 3 outputs, yes?
Done
Could you give a brief explanation of the trades and temperatures arrays? Just so people get what it is.
Done
I think buyIndex and sellIndex also need explanation.
Done... Not too sure of the quality of the explanations :-)
If you run npm run docs:dev you can serve the doc locally and see exactly how it is rendered. I use Vuepress 2 for the docs. >There are some very useful custom containers that you could use: https://v2.vuepress.vuejs.org/reference/default-theme/markdown.html#custom-containers For example, you could use the details custom container instead of the details html tag to get a better styling.
Downloaded and tested. Containers look great, just hope they show up on Github :-)
In the examples you are using the blockquote notation. These are not quotes, so maybe some other means could be better. >Some places you could just use normal bullet points, other places maybe plain text is better. Run locally to see how it renders. Buy the way, I did not know about the blockquote in Markdown, so thank you for teaching me :-)
Todo: Will give it a go and se if I find a nice way to do this. Not too versed in MD.
Please add links to the examples in the examples/README.md file
Done
Not sure I got everything here. I managed to write all the feedback for then loosing it by a mistake, so I had to write everything again. I may have forgotten something... Hehe... I used Ctrl-C regularly as a back-up while writing this. :-) Feel free to comment if you find/remember something more.
Very good work!
I have been trying to get the TeX rendered in Vuepress, but currently without success. Not yet found a plugin for Vuepress 2. I am sure it is possible. Do you have any idea?
About outputting a "schedule", I agree with you that we should not output every minute. However, the data is not changing every minute, maybe it could be possible to output a schedule with only the entries when it is actually changing? We don't need to do it immediately.
Anyway, do you feel the code is ready to be merged, or is there anything else you like to do?
I have been trying to get the TeX rendered in Vuepress, but currently without success. Not yet found a plugin for Vuepress 2. I am sure it is possible. Do you have any idea?
Hmm... I see that it is not rendering correct on Github either. I will see if I can figure it out/change to another approach.
About outputting a "schedule", I agree with you that we should not output every minute. However, the data is not changing every minute, maybe it could be possible to output a schedule with only the entries when it is actually changing? We don't need to do it immediately.
The "trades" data is intended for this. It shows the times at which the system moves from maintaining the current temperature, to either reducing or increasing it.
Anyway, do you feel the code is ready to be merged, or is there anything else you like to do?
Hehe... I would say that it is ready for beta testing? :-) If someone could try to implement it on their system and then give feedback on the documentation and functionality?
Adds a new strategy and some documentation to the repository.
Todo: Might want to add some warning badges :-)