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 #27 Path limit issues #417

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from
Open

Conversation

pcboy
Copy link

@pcboy pcboy commented May 28, 2024

As I encountered issue #27 , I tried to fix it.

I made a glorified search and replace in a script called ./autofix_paths.rb that we can remove after the PR is reviewed.

The resulting DSSAT seems to be working for me at the moment, with the path issue fixed. I ran a batch file and got my results.

Notes:

  • The changes I made are from tag v4.7.5.42 and not from v4.8.2.10 (this has been rebased with develop now). The reason is the latest tag does not compile for me on Linux v4.8.2.10 not compiling with gfortran on linux #416 . If that issue gets fixed, I can likely re-run my script.
  • Why are you * text eol=crlf automatically in the gitattributes? This is something that should be handled by the editors instead. And I'm sure even on windows, editors should be able to handle \n for newlines? This setup caused me issues on my side.
  • This is a bastard search and replace, I did my best as to be fairly specific, but this is likely that some variables are using more stack space than necessary.
  • I can't find any tests in this project at the moment? So I 'm not sure how to make sure I didn't break anything.

@pcboy pcboy force-pushed the fix-path-limit branch from 63c16d8 to 0f8a496 Compare May 29, 2024 07:02
@pcboy
Copy link
Author

pcboy commented May 29, 2024

After fixing the compilation of the develop branch on linux in #418 , I was able to rebase this branch with develop.

This still seems to be working. I got my results.

@chporter chporter requested a review from fabiooliveira72 May 29, 2024 21:56
@chporter
Copy link
Contributor

@fabiooliveira72 Can you put this in your queue for testing?

@fabiooliveira72
Copy link
Contributor

Hi @chporter!

Yes, for sure. I saw the PR. I will pass through this soon and test it.

I have another trip coming next week and Im finishing up the workshop things. Ill

Thank you.

Fabio Oliveira

@pcboy
Copy link
Author

pcboy commented Jun 20, 2024

Since new stuff was merged to the develop branch, I rebased.

@pcboy
Copy link
Author

pcboy commented Jul 17, 2024

@fabiooliveira72 Is there any ETA for this testing?

Just wondering because if more things get merged then I have to fix conflicts.

Note that my dssat nix package got merged in Nixpkgs so anyone can now run dssat with no compilation/dependencies needed with nix run github:nixos/nixpkgs/nixos-unstable#dssat , this includes my patch for the path limit in this MR.

@fabiooliveira72
Copy link
Contributor

fabiooliveira72 commented Jul 24, 2024

Hi @pcboy!

Thanks for your patience and checking this.

Yes, please keep updated your branch and resolve the conflicts. We need to delay the merge of this pull request because this is a major change that affect all the model. We have an upcoming DSSAT Release that we need to keep stable. I also have some updates for this pull request that I will merge with mine. The problem is the testing of all this.

But keep this branch updated. After the release we can get back to this.

Thank you so much!

@pcboy
Copy link
Author

pcboy commented Dec 27, 2024

@fabiooliveira72 Any news?

Clearly it's not really possible to make any significant changes to DSSAT since there are no unit tests.

Is there any plan to have some sort of test suite? Without a test suite, how do you even know DSSAT is working today I wonder?

@chporter
Copy link
Contributor

chporter commented Dec 27, 2024

@pcboy Unit tests would be nice but completely inadequate for a large integrated model like DSSAT where a change to one module may impact other modules. We employ a whole-model, comprehensive, integrated approach to testing changes using a set of DSSAT input files which use the full range of options and capabilities, testing against measured data as well as previous stable model outputs.

@pcboy
Copy link
Author

pcboy commented Dec 27, 2024

@chporter Sure, that's an integrated test then. I was saying unit tests to refer to any automated test.

So where is this automated test suite that shows that the results are correct and don't break over time?

@chporter
Copy link
Contributor

@pcboy Right now our testing is not automated. It's done by individuals with some offline tools and data. We really need a continuous integration system in github. Are you interested in helping to develop one?

@pcboy
Copy link
Author

pcboy commented Dec 28, 2024

@chporter I'm not really having any personal usage of DSSAT, so I'm not really willing to spend much personal time on it to be honest. I'm also far from understanding the details about this tool since I'm not a researcher, so likely not the best for this.

With that being said, this shouldn't be too complex to start. You simply need to do what you are doing manually, in code. It can be as simple as:

  • A folder tree like:
tests
└── test1
    ├── expected_outputs
    │   └── output1.txt
    └── inputs
        ├── DSSBatch.v48
        └── weather.WTH
        └── etc.etc

And test suite basically just run each test with the inputs and compare the result of dssat to the expected output.
I'm willing to help doing the "framework" to do that, then you only need to put the input data and expected output in the correct folders.
As soon as you have that, you at least can make sure that any change you do in the code, is not affecting the results, and if they do, you would have to manually change the test's expected output anyway. And that change would be reviewed in a PR.
Of course this is not perfect, but this would be a good first start and better than current situation. If something like this was present today, this PR would already be merged.

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.

3 participants