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

Auto assign basin projection attributes and basin center #154

Merged
merged 23 commits into from
Apr 30, 2020

Conversation

micahjohnson150
Copy link
Contributor

In #100 and #97 we are looking to remove config file options in favor of obtaining them from either the topo.nc file or from auto calculation specifically relating to the basin's center (lat lon) and the projection attributes (zone number letter etc).

I added a simple test confirming the lat lon calculations as well to test_load_data.py

This will impact the net_radiation gold file as the new lat lon is slightly different. So review the code and then I will do the analysis showing the changes to the gold files.

…all lat log references to the topo class, retrieving zone number from the topo.nc, added a northern hemisphere config option to avoid rewriting topos files since they dont have the zone number. Removed a couple deprecated recipes relating to topo
…all lat log references to the topo class, retrieving zone number from the topo.nc, added a northern hemisphere config option to avoid rewriting topos files since they dont have the zone number. Removed a couple deprecated recipes relating to topo
…all lat log references to the topo class, retrieving zone number from the topo.nc, added a northern hemisphere config option to avoid rewriting topos files since they dont have the zone number. Removed a couple deprecated recipes relating to topo
…all lat log references to the topo class, retrieving zone number from the topo.nc, added a northern hemisphere config option to avoid rewriting topos files since they dont have the zone number. Removed a couple deprecated recipes relating to topo
@micahjohnson150 micahjohnson150 changed the title Auto assign basin proejction attributes and basin center Auto assign basin projection attributes and basin center Apr 15, 2020
…e config file. Updated the config file and autocalculated the basin_lat long. Fixed #100 and #97
@micahjohnson150
Copy link
Contributor Author

The builds wont pass here until we adjust the gold files. I confirmed the failures are due to net solar in the local_hrrr and the normal RME run.

Copy link
Contributor

@scotthavens scotthavens left a comment

Choose a reason for hiding this comment

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

Looks good, mainly organizational changes that are requested.

requirements.txt Outdated Show resolved Hide resolved
smrf/data/loadGrid.py Outdated Show resolved Hide resolved
smrf/utils/utils.py Outdated Show resolved Hide resolved
tests/test_load_data.py Outdated Show resolved Hide resolved
tests/test_load_data.py Outdated Show resolved Hide resolved
tests/test_load_data.py Outdated Show resolved Hide resolved
…ass, moved topo tests to its own file test_topo, removed all the class attributes in loadGrid so they directly refer to the topo class, added a test to insure these attributes stay.
smrf/data/loadGrid.py Outdated Show resolved Hide resolved
@micahjohnson150
Copy link
Contributor Author

Ok I finally have the gold analysis. I took a little side journey to add to Joes gold diff script. I just always assumed you couldn't switch branches like that while running your code. It turns out to be a super useful for thing for me in a quite a few other areas. So I have added it all to a package called goldmeister in our group.

As for my changes here you go.

The new calculation for the basin lat and long is slightly different than what we see in the config file. So there will be net solar changes. Below are my changes to the gold files as the gold config files for hrrr and station data are committed currently.

projections_updates >> tests/RME/gold/gold_config.ini

net_solar nc_net_solar

projections_updates >> tests/RME/gold_hrrr/gold_config.ini

Note, included here are the other changes reported in addition to net_solar but I believe these are the discrepancies we have seen from computer to computer for HRRR data.

net_solar nc_net_solar
precip_temp nc_precip_temp
thermal nc_thermal
vapor_pressure nc_vapor_pressure

I wanted to be sure in my changes so I went back to the master branch and changed the config file to the lat long that I am calculating to see the changes were identical.

master >> tests/RME/gold/gold_config.ini [MODIFIED]

net_solar nc_net_solar

master >> tests/RME/gold_hrrr/gold_config.ini [MODIFIED]
net_solar nc_net_solar
precip_temp nc_precip_temp
thermal nc_thermal
vapor_pressure nc_vapor_pressure

@scotthavens
Copy link
Contributor

It appears that changes to precip_temp, thermal and vapor_pressure are just float point precision?

@micahjohnson150
Copy link
Contributor Author

@scotthavens yep, that was my interpretation. The hrrr tests when not on the host machine that made them only tests to 0.003 precision. I just didn't know what the protocol is for this situation. I almost left them out but I wanted to show them incase someone disagreed with them being float precision.

@micahjohnson150
Copy link
Contributor Author

Actually now that I say this, I am assuming you made the gold images for hrrr and you machine is the host? If thats the case and we want to keep that, we will want you to commit the gold file changes when we decide to do so.

@scotthavens
Copy link
Contributor

@jomey was the last one to change the gold files I believe but this is just a problem that we will have to figure out.

To take host machines out of it, maybe we build the gold files with the docker image?

@jomey
Copy link
Collaborator

jomey commented Apr 22, 2020

@jomey was the last one to change the gold files I believe but this is just a problem that we will have to figure out.

To take host machines out of it, maybe we build the gold files with the docker image?

I used the docker image to rebuild the gold files.

@micahjohnson150
Copy link
Contributor Author

Ok I am doing better now. Fewer tiny changes. Thanks for the tip. The following results are comparison from my branch projections update to master. The data was remade using the docker.

Here it is for the station gold:
net_solar nc_net_solar

Here it is for the HRRR data
net_solar nc_net_solar
thermal nc_thermal

@scotthavens
Copy link
Contributor

Looks a lot better with just net_solar. A little odd that thermal is different but it's all in the noise.

Copy link

@robertson-mark robertson-mark left a comment

Choose a reason for hiding this comment

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

Looks good to me, only the one very minor possible cleanup.

@micahjohnson150 micahjohnson150 merged commit a09ad0d into master Apr 30, 2020
@micahjohnson150 micahjohnson150 deleted the projections_update branch April 30, 2020 16:51
jomey added a commit to USDA-ARS-NWRC/awsm that referenced this pull request May 8, 2020
PR USDA-ARS-NWRC/smrf/pull/154 removed the basin lat and lon from the
configuration file. These changes reflect the necessary updates to AWSM.
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.

4 participants