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

Closes #61: Added Rs dataset, updated README, and added pull request template #64

Merged
merged 3 commits into from
Jul 19, 2024

Conversation

msteckle
Copy link
Collaborator

  • First, I added the Rs dataset as requested in Issue Data for a Soil Respiration (Rs) Confrontation #61.
  • I also updated the README a little bit. Check it out and see if you approve? I just wanted to add a little detail. I find myself digging for the links I provided in response to the closed issue Questions about contribute to ilamb-data collection #60 and thought it might be nice to add? I did also do some grammar things you can ignore, but just in case.
  • I also made a super campy pull_request_template in .github/. It probably has too much on it, and I haven't checked to see if it formats correctly. But I figure better than nothing! Let me know what you think about that too!

Note: Oops, I forgot to remove the extra files via the .gitignore. Will remove those in next commit.

Pic of the Rs dataset:
download

@msteckle msteckle requested a review from nocollier July 10, 2024 21:07
@msteckle msteckle self-assigned this Jul 10, 2024
@msteckle msteckle linked an issue Jul 10, 2024 that may be closed by this pull request
SRDB/convert.py Outdated
target_units_obj = cfunits.Units(target_units)

# Handle specific cases with intermediate conversion steps
if original_units == 'g m-2 y-1' and target_units == 'kg m-2 s-1':
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am a little unclear on why this "if" block is necessary. I don't use cfunits but cf-units, I know it is confusing. But both wrap UDUNITS2. Are they unable to directly convert g m-2 y-1 to kg m-2 s-1?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, it gave me an error, which is why I threw in the if-statement. I can give cf-units a try! I didn't realize there were two different ones... :-)

Copy link
Collaborator

Choose a reason for hiding this comment

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

At one point I had some PR's to them and they didn't respond and I made the switch. I wonder if it is because y is not a unit in the UDUNITS database. You may try y-1 --> yr-1 and then see if the package just converts the units?

Copy link
Collaborator

Choose a reason for hiding this comment

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

What do you think of:

original_units = original_units.replace("y-1","yr-1")

and then your conform line?

@msteckle msteckle requested a review from nocollier July 19, 2024 15:36
@nocollier nocollier merged commit 33c07da into master Jul 19, 2024
@msteckle msteckle deleted the 61-data-for-a-soil-respiration-rs-confrontation branch July 31, 2024 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Data for a Soil Respiration (Rs) Confrontation
2 participants