search

openclimatefix / PVNet

PVnet main repo
MIT License
17 stars 3 forks source link

merge pvnet and pvnet summation #77

Closed peterdudfield closed 1 year ago

peterdudfield commented 1 year ago

I saw that pvnet_summation installs pvnet and pvnet install pvnet_summation. This seems a bit circular? I wonder if bring in all the pvnet summation stuff into pvnet here might be a good thing to do?

could pvnet_summation be a sub model/folder in pvnet

peterdudfield commented 1 year ago

what do you think @dfulu?

dfulu commented 1 year ago

We could, but I think it may be cleaner to keep them separate. They have different sets of config files, different models, different datamodules, and different training scripts. So it would mean having pairs of subdirectories like config/pvnet_summation and config/pvnet and so on if they were combined into the same library.

Currently pvnet only imports the summation model for the app. If we wanted to keep the imports unidirectional, the app could be moved to PVNet summation?

peterdudfield commented 1 year ago

hmmm, interesteding, the app could be moved to pvnet_summation. And then run it off there?

Could also do a pvnet-liv that both pvnet load shared componetns from.

Hmm, im not sure what the right thing here is to do

dfulu commented 1 year ago

For posterity: We decided to create a new repo pvnet_app to run the production app. This allows us to keep separate the experimental and production code and keep the PVNet summation and PVNet separate as well.

Keeping the two model libraries separate seemed cleaner to avoid confusion in having two sets of configs, models, datamodules etc. Removing the app to its own separate repo means that now pvnet does not depend on pvnet_summation. This fixes the circular imports