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

Update to crops definition function to allow for "both" #9053

Closed

Conversation

lilyclements
Copy link
Contributor

We want to have the option to see the results with "Start Check" both TRUE and FALSE in the same table (as two columns)

This PR does this, so now start_check takes "yes", "no", and "both" as arguments.

start_check == "yes" works how start_check == TRUE used to work
start_check == "no" works how start_check == FALSE used to work
start_check == "both" gives results for start_check == TRUE and start_check == FALSE. This therefore gives two columns - instead of overall_cond, which is given for the above two options, this gives overall_cond_with_start and overall_cond_no_start.

This is for use in EPICSA where we want to have stored a whole set of summaries to be calculable from a saved file (i.e., the resulting "crops_def" file here)

This is linked to issue #9052. I suggest that whoever implements #9052 reads in these changes (this should not be merged without amendments made to the dialog because of the changes in start_check)

Want to have the option to see the results with "Start Check" both TRUE and FALSE in the same table (as two columns)
N-thony
N-thony previously approved these changes Jul 8, 2024
@Vitalis95
Copy link
Contributor

@N-thony , could you please merge this PR, the updated function is needed in #9053

@N-thony
Copy link
Collaborator

N-thony commented Jul 8, 2024

@N-thony , could you please merge this PR, the updated function is needed in #9053

@Vitalis95 I have already approved, we can merge this once @rdstern approves too.

@lilyclements
Copy link
Contributor Author

@N-thony @Vitalis95 merging this in would mean the crops probabilities dialog does not work until the appropriate changes are made in the dialog. I suggest this not merged into the master branch but instead @Vitalis95 take over this branch and make in it your changes to the dialog

@Vitalis95
Copy link
Contributor

@lilyclements , I get the following error from this PR when importing data

image

@lilyclements
Copy link
Contributor Author

@Vitalis95 that's a pretty major error! Thanks - should be fixed now, oops.

@rdstern
Copy link
Collaborator

rdstern commented Jul 9, 2024

@lilyclements and @Vitalis95 I am not sure if I am needed here? I think if @lilyclements and @N-thony approve, then this can be merged, though perhaps it is being incorporated elsewhere, and can then be closed?
I take the opportunity to note that perhaps my addition of the seq option in the dialog could help, if added when there are just 3 numbers in the control, and the third is smaller than the second. If so, then some examples are also needed in the pull-down.

@lilyclements
Copy link
Contributor Author

@rdstern I did wonder about using the sequence ideas from the seq dialog. David didn't seem to jump onto that idea -- I think because we just want to offer the same "options" each time for our EPICSA users, but perhaps I am mistaken. It would be good for us to clarify that.

I am happy to close this once @Vitalis95 has merged into his own branch?

@Vitalis95
Copy link
Contributor

@lilyclements , there is this error when you run the updated function

image

Here is the code;

data_book$crops_definitions(data_name="ghana", year="year", station="station", rain="rainfall", day="doy", plant_days=c(120), 
plant_lengths=c(120), rain_totals=c(600), start_day="start_rain", end_day="end_rains",
 season_data_name="ghana_by_station_year", start_check="yes")

@lilyclements
Copy link
Contributor Author

@Vitalis95 thanks. this should be fixed now

@N-thony
Copy link
Collaborator

N-thony commented Aug 26, 2024

@Vitalis95 thanks. this should be fixed now

@Vitalis95 do you still need this PR?

@Vitalis95
Copy link
Contributor

@N-thony , we no longer need this

@N-thony
Copy link
Collaborator

N-thony commented Aug 26, 2024

@N-thony , we no longer need this

can you close it?

@Vitalis95 Vitalis95 closed this Aug 26, 2024
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.

4 participants