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

Solar coords #190

Open
wants to merge 24 commits into
base: main
Choose a base branch
from
Open

Solar coords #190

wants to merge 24 commits into from

Conversation

felix-e-h-p
Copy link
Contributor

@felix-e-h-p felix-e-h-p commented Feb 26, 2025

Decoupled solar coords - potential inclusion for further tests here, yet not sure if entirely necessary - updated tests relative to changes none the less

Relates to #179

@felix-e-h-p felix-e-h-p marked this pull request as ready for review February 26, 2025 08:11
@felix-e-h-p felix-e-h-p requested a review from Sukh-P February 26, 2025 08:11
Copy link
Member

@Sukh-P Sukh-P left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making a start on this, think the logic of the code needs to be double checked and still more decoupling of the solar coordinates from GSP/site

@Sukh-P
Copy link
Member

Sukh-P commented Mar 7, 2025

@felix-e-h-p I think this looks good and pretty much ready to go but I think that perhaps it still might break things on the site side, namely I am not sure this function will work now, so would it be possible to add a test which generates a site sample with the new solar configuration and then pass it through this function to see whether it gives you a numpy sample with the correct keys (namely a key that relates to solar), I think it won't and that function will need some tweaking, thanks!

Also there definitely will need to be some changes in PVNet alongside this as you can see here https://github.com/openclimatefix/PVNet/blob/main/pvnet/models/multimodal/multimodal.py#L347-L351 it relies on a naming convention that we want to move away from

@felix-e-h-p
Copy link
Contributor Author

Cheers for the feedback.

pvnet_uk.py now has a solar config object with time parameters instead of borrowing from GSP settings (also updated relevant variable namings RE prefix).

site.py likewise has a solar config object with related timing parameters - hopefully properly handling site timestamp dimensions. t0 parameter is now passed from _get_sample method to process_and_combine_site_sample_dict so that the initial timestamp can be used for calculating the solar position timestamps accurately (no mismatch here).

Site tests updated accordingly - two additional tests.

@dfulu
Copy link
Member

dfulu commented Mar 10, 2025

@felix-e-h-p thanks for the changes.

There is some duplication in these lines which should be removed. The code doesn't use datetimes, lon, or lat created in the if target_key == "gsp" block

Also whilst we're here, it might be best to strip out the target_key param from that process_and_combine_datasets() function. That param is no longer used

)

# Ensure matching of site time dimension length
solar_datetimes = solar_datetimes[:site_time_length]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This couples the solar coords to the target datetimes. It also means the solar config doesn't reflect the datetimes of the solar coords. This should be removed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wasn't sure with this, cheers. Now have created a dimension for solar position data - ensures solar coords are determined solely by config params.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants