Navigation Menu

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

Added new options to Evapotranspiration dialog #7958

Merged
merged 17 commits into from Dec 2, 2022

Conversation

MeSophie
Copy link
Contributor

I made some modifications on this Dialog according to @rdstern comment on PR#7611.
@Patowhiz, @N-thony , @AmsaleEjigu please can you have a look.

Copy link
Collaborator

@rdstern rdstern left a comment

Choose a reason for hiding this comment

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

@MeSophie that's a good start. Could you also look at the other suggestions I made in the layout in #7611.
Then, in addition, what I haven't done is to make much use of the dialogue. I suggest you also do this with the ICRISAT data and with other data sets. And that could be in parallel with the cosmetic layout improvements.
That should make it easy to write the help, and possibly produce a video on the use of the dialogue.
To start I wonder what examples are supplied with the package itself. Then there is the bohicon data in the Instat library climatic guide data sets (omit the top about 13 lines). And you should download the ICRISAT data I mention in 7611.

Those examples will (I hope) be fairly trivial to run, but let's see.

Then we need to see how to use it when there are missing values and when elements are missing. I think the function will cope, but this will need investigation. I suggest it will be efficient for you to start with the documentation from the package and we may eventually have to look at the function itself - though I hope now!

@MeSophie
Copy link
Contributor Author

@rdstern Please can you go and look the comment that I made on PR#7611. Thanks

@MeSophie
Copy link
Contributor Author

I noticed that after filling in the 4 main parameters, the Ok button is active but if we delete any of these parameters, the Ok button is still active, so I have corrected this problem.

image

Copy link
Collaborator

@rdstern rdstern left a comment

Choose a reason for hiding this comment

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

@MeSophie I am now c learer on the possible improvements in layout.
image

In the image above
a) Change the label Radiation to Solar.
b) In the pull-down, change Data to Radiation
c) Don't have the Wind in a block. Instead just have the existing Checkbox with label Wind Speed, directly underneath the Solar label. Default - as now, is checked and the receiver is there. If unchecked, then - as now - the receiver is no longer visible.
d) Please check whether the options for missing are all the same for the second main radio-button. If so, then just improve the spacing a bit.

I then tried with the Bohicon data - here it is. It seems to work!
bohicon.zip

Finally I noticed that the humidity data is not yet in our defined climatic data, so that's a new issue. But let's see if this one can be merged first - and then we are ready to test it!

@MeSophie
Copy link
Contributor Author

Please check whether the options for missing are all the same for the second main radio-button. If so, then just improve the spaci

@rdstern The missing options contained in the missing options sub-dialogue are common to both radio buttons. I change the Radiation label to Solar but is still Radiation. So @Vitalis95 will change also the database.

@rdstern
Copy link
Collaborator

rdstern commented Nov 22, 2022

@MeSophie thank you for the explanation about Solar.
There is now one further (welcome) change I am afraid!
image

The change is in the Define Climatic data, where the humidity variables are now defined - and this means that they can all be included automatically when the dialogue is loaded. And that will make the method simpler to use.

So can you check how the existing elements are added automatically into the dialogue, and extend that now to the humidity variables. You may have to wait till the Define Climatic Data dialogue is merged.

@MeSophie
Copy link
Contributor Author

@MeSophie thank you for the explanation about Solar. There is now one further (welcome) change I am afraid! image

The change is in the Define Climatic data, where the humidity variables are now defined - and this means that they can all be included automatically when the dialogue is loaded. And that will make the method simpler to use.

So can you check how the existing elements are added automatically into the dialogue, and extend that now to the humidity variables. You may have to wait till the Define Climatic Data dialogue is merged.

@rdstern I'm working on it. All climatic data are include automatically for Bihicon data but for Patancheru there is small issue with radiation.

@MeSophie
Copy link
Contributor Author

MeSophie commented Dec 1, 2022

@lloyddewit , @rdstern this PR is ready to review.
There is still the radiation Solar parameter which does't fill automatically but @N-thony suggest that after merge the PR we can create and issue for this.

rdstern
rdstern previously approved these changes Dec 1, 2022
Copy link
Collaborator

@rdstern rdstern left a comment

Choose a reason for hiding this comment

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

GReat

@lloyddewit
Copy link
Contributor

@rdstern if you approve, then I think we can merge, thanks

@ChrisMarsh82 The compile checks are failing (see screenshot below). However, they are failing on the compilation error that was fixed in PR #7997 . Master now builds. I don't understand why this PR tries to merge/build against a previous master commit. I reran the GitHub build checks in 4 other PRs and they all passed so I don't believe there is a general problem. I propose to merge this PR anyway. What's your view?

image

@lloyddewit
Copy link
Contributor

I reran the GitHub build checks in 4 other PRs and they all passed so I don't believe there is a general problem.

@ChrisMarsh82
I just found that the build checks also fail in PR #7930 for the same reason. Not sure why.

@lloyddewit
Copy link
Contributor

@ChrisMarsh82 Your theory was correct! thank you!
Checks seem OK now. I'll merge this PR tomorrow.

Copy link
Collaborator

@rdstern rdstern left a comment

Choose a reason for hiding this comment

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

@MeSophie seems to work fine - even adding the humidity! But not the sunshine - so that can become an issue. Perhaps together with the 3rd method to add!

@lloyddewit lloyddewit changed the title Added Missing Options on Evapotranspiration Dialog Added new options on Evapotranspiration dialog Dec 2, 2022
@lloyddewit lloyddewit changed the title Added new options on Evapotranspiration dialog Added new options to Evapotranspiration dialog Dec 2, 2022
@lloyddewit lloyddewit merged commit 5552f5b into IDEMSInternational:master Dec 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants