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

fix: adjust alignment of values to soil headers #33

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

aarojas20
Copy link
Contributor

@aarojas20 aarojas20 commented Jun 15, 2022

This PR address the following issue: #32

  • adjust the logic for justification of values wrt to their type, float vs non-float
  • ensure the SLB header ends on column 7 to align with its values that also end on column 7.

In the screenshot below, I compare the soil.SOL files from the original (left) to the one that this PR generates (right). The alignment of the variables to the headers are improved, albeit not perfect. However, I also verified that the results from the DSSAT simulation were the same.

Screen Shot 2022-06-15 at 1 18 11 PM

DSSAT run with the original soil.SOL file from DSSAT.
Screen Shot 2022-06-15 at 1 36 37 PM

DSSAT run with newly created soil.SOL file from this fix-soil-write branch.
Screen Shot 2022-06-15 at 1 22 55 PM
Screen Shot 2022-06-15 at 1 24 23 PM

Second test-case

Separately, I also verified that the writing of the weather file was not negatively impacted. I was able to verify that the results of a simulation were the same. In this test case, I did not change the SOL file, I used the one that DSSAT provided. I opened up a weather file and re-wrote it with this branch, and re-ran a simulation. The results were the same.

@aarojas20 aarojas20 marked this pull request as ready for review June 15, 2022 18:17
@julienmalard
Copy link
Owner

Hello,

Many thanks for this pull request!

Looking at the Travis CI tests I see a few errors but I'm not sure whether they're really errors with this PR or with DSSAT files in general. A few errors seem to be minor issues in the parsing of values, for instance on lines 979/983. Here for example the PR branch gives "PN001511.8", while the reference was "PN0015". However, this seems to be a malformatted line in the PNGRO047.CUL file anyways (last line). In such a case, I'm not sure what the "correct" behaviour should be - skipping the line? Refusing to parse?

Would you be able to run the tests locally and see, in each case, whether the discrepancy between reference and real value is due to a mistake in the reference or the actual value? If there error is in the reference, you can erase the ref file and rerun the tests, at which point traDSSAT will use the actual value generated by your branch as the future "correct" reference. (If you commit the new reference file to git the tests should then pass on TravisCI.)

Many thanks for your help and please let me know if this is clear,
Julien

@aarojas20
Copy link
Contributor Author

Thank you for the feedback! I'll take a look at those tests.

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.

2 participants