search

NREL / nsrdb

NSRDB data processing pipeline. Includes satellite data assimilation, cloud property prediction and gap-filling, radiative transport modeling, and data collection.
https://nrel.github.io/nsrdb/
BSD 3-Clause "New" or "Revised" License
7 stars 0 forks source link

Use FARMS-DNI #58

Closed xieyupku closed 1 year ago

bnb32 commented 1 year ago

@grantbuster are the files used in the all sky tests precomputed data? The naming suggests theyre surfrad data but Yu says they also include nsrdb outputs.

grantbuster commented 1 year ago

@grantbuster are the files used in the all sky tests precomputed data? The naming suggests theyre surfrad data but Yu says they also include nsrdb outputs.

/data/validation_nsrdb/nsrdb_surfrad_2000.h5? This is NSRDB baseline output data processed for the surfrad locations. Confusing naming, i know. The ground measurement data is in /data/validation_surfrad/tbl_2017.h5. Feel free to rename / move directories.

bnb32 commented 1 year ago

@grantbuster are the files used in the all sky tests precomputed data? The naming suggests theyre surfrad data but Yu says they also include nsrdb outputs.

/data/validation_nsrdb/nsrdb_surfrad_2000.h5? This is NSRDB baseline output data processed for the surfrad locations. Confusing naming, i know. The ground measurement data is in /data/validation_surfrad/tbl_2017.h5. Feel free to rename / move directories.

Ok cool. @xieyupku you can use these files to setup the new tests.

grantbuster commented 1 year ago

We might not need new tests. The new outputs shouldnt differ that much from the legacy runs, should they? Also if you do have to revise tests, there's no need to have 20 years of test data. That was dumb on my end.

bnb32 commented 1 year ago

We might not need new tests. The new outputs shouldnt differ that much from the legacy runs, should they? Also if you do have to revise tests, there's no need to have 20 years of test data. That was dumb on my end.

I suppose we could just revise the error thresholds in the existing tests. The farms update produces results outside of the current 10% threshold.

xieyupku commented 1 year ago

The GHI from the new code is the same as the previous FARMS. The DNI and DHI are different. I think we could only compare GHI in the test for all_sky.py. Or we could also make sure the new DNI is in a reasonable range compared with the previous computation. Comparing with surface data is too complicated for this test I think.

We might not need new tests. The new outputs shouldnt differ that much from the legacy runs, should they? Also if you do have to revise tests, there's no need to have 20 years of test data. That was dumb on my end.

bnb32 commented 1 year ago

@xieyupku Commit messages should say what is being committed - e.g. what was changed, what was added, etc. Having 'FARMS-DNI' for every commit doesn't tell us anything.

bnb32 commented 1 year ago

@xieyupku this call to disc https://github.com/NREL/nsrdb/blob/8a5501190119f9a1dfe3980991c6d99c79c5c274/nsrdb/all_sky/all_sky.py#L186 can go under here https://github.com/NREL/nsrdb/blob/8a5501190119f9a1dfe3980991c6d99c79c5c274/nsrdb/all_sky/all_sky.py#L193 since disc is only needed with farmsdni=False.

grantbuster commented 1 year ago

hey @bnb32, can you add the disc_on kwarg to two more places to make it fully integrated into the CLI? Sorry, i know the cli is a tangled web.

https://github.com/NREL/nsrdb/blob/34ec0df68b11fecc376e16310f4a094da66f17cc/nsrdb/cli.py#L577 https://github.com/NREL/nsrdb/blob/34ec0df68b11fecc376e16310f4a094da66f17cc/nsrdb/cli.py#L617

bnb32 commented 1 year ago

Looks good, just need to fix the tests and then lets merge

Yeah, Yu is still working on that.