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

Minor updates to allometry functional unit tests #656

Merged
merged 3 commits into from
Jun 22, 2020

Conversation

rgknox
Copy link
Contributor

@rgknox rgknox commented May 18, 2020

Description:

This set of changes makes the allometry unit tests Python 3 compliant, and adds some minor syntax changes and small tweaks to the scripting and analysis.

Collaborators:

@adrifoster @jkshuman @KatieMurenbeeld

Expectation of Answer Changes:

This should have no impact on FATES

Checklist:

  • I have updated the in-code documentation .AND. (the technical note .OR. the wiki) accordingly.
  • I have read the CONTRIBUTING document.
  • FATES PASS/FAIL regression tests were run
  • If answers were expected to change, evaluation was performed and provided

Test Results:

The changes to the FATES non-unit testing code is only comments. The unit testing framework is a test itself, and these changes are designed to just clean up the tests and make them easier to use.

CTSM (or) E3SM (specify which) test hash-tag:

CTSM (or) E3SM (specify which) baseline hash-tag:

FATES baseline hash-tag:

Test Output:

@rgknox rgknox added enhancement PR status: Ready This PR is ready for integration after having passed through peer review and final testing. labels May 18, 2020
@glemieux
Copy link
Contributor

@adrifoster @jkshuman @KatieMurenbeeld I talked with Ryan about review of this PR. We agreed that although it's a pretty simple change, we wanted to give y'all the opportunity to check it out and suggest changes or improvements (since it's not a time critical PR). Please let us know if you have any suggested changes.

@jkshuman
Copy link
Contributor

@glemieux @rgknox thank you, and sure thing will look it over this week. I was using the tests this weekend, so interested to look over the new improved version.

@glemieux
Copy link
Contributor

glemieux commented May 18, 2020

@rgknox for future unit test updates, what do you think about producing a subset of tests results to share in the PR? The better verification is to get others to use it, but maybe having a small suite of tests to kick off would be good to verify things are working for others?

@rgknox
Copy link
Contributor Author

rgknox commented Jun 21, 2020

I think a suitable time as elapse for others testing this code to chime in. I propose we move ahead, integrate, and get this off the growing list of PRs.

@glemieux glemieux merged commit e4b344a into NGEET:master Jun 22, 2020
@jkshuman
Copy link
Contributor

jkshuman commented Jun 22, 2020

@rgknox I tested this and it now works with Python3. I got an error with one section, but that may because of the file I am using. Let me know if you want to investigate/talk about it.

drive_allomtests.py:474: RuntimeWarning: divide by zero encountered in double_scalars
  blmax_o_dbagwdd[ipft,idi]  = blmaxi[ipft,idi-1]/(cdbagwdd.value)

I also got these errors, but that may be from running on Hobart?

libGL error: No matching fbConfigs or visuals found
libGL error: failed to load driver: swrast

@rgknox
Copy link
Contributor Author

rgknox commented Jun 22, 2020

@jkshuman , regarding the first issue, I think that should render a missing point on the plots, which should be useful information in error checking. If the line suddenly stops, something is wrong with the function somewhere.

Regarding the second, yeah, not sure. Sounds like the python plotting functions want access to the low level graphics library libgl, but can't find it. There should be module dependencies on the machine that make sure libgl is loaded. Maybe load it manually, I forget how hobart handles loading software.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement PR status: Ready This PR is ready for integration after having passed through peer review and final testing.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants