jranalli / solarspatialtools

Library of tools used to support spatial analyses of solar energy
https://solarspatialtools.readthedocs.io
BSD 3-Clause "New" or "Revised" License
1 stars 3 forks source link

JOSS review: repo comments #14

Closed kbonney closed 1 month ago

kbonney commented 2 months ago

Hi @jranalli, I was able to install the package and execute all of the tests and notebooks without major issue. Below are a few comments regarding the repository:

Happy to clarify/assist with any of these items.

jranalli commented 2 months ago

Since these changes weren't that big structurally, I made them in the same pull request as responding to Issue #11 (pull request #13). I took care of the easy ones, and have one minor point left.

1) Added, along with instructions to install jupyter for the demos in the README 2) Modified the action to run them all at once. 3) You're correct, they should be independent. The assumption to do this is that the plant has only isolated mislabeling and that has been consistent with the operational data experience of using the method. In such a case, we've found that "good enough" CMVs can be identified even when keeping the scrambled data, because the errors introduced by false positions end up averaging out as you incorporate the large plant. But it is in principle a limitation of the method. I guess you could add the CMV calculation to the iterative loop of correcting the positions, but we haven't had data that would require that to really test out whether it would work. Nonetheless, I've added a little more notation there to clarify. 4) I added links to the Readme and the github About box, but am not done here. I still need to look into how to use nblink to modify the demo locations.

jranalli commented 2 months ago

OK, got nblink figured out and moved the demos back to the parent directory in the latest push to the PR. Demos appear to build correctly in the PR-level readthedocs build. So I would propose that PR #13 addresses all these concerns per your approval.

kbonney commented 2 months ago

@jranalli, thanks for addressing these comments. #13 looks good to me.