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

Merge Staging to Master #28

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

Merge Staging to Master #28

wants to merge 29 commits into from

Conversation

ahmedjoubest
Copy link
Collaborator

I reviewed the R Package GitHub Repository management and made the following updates:

Created a staging branch (https://github.com/CIAT/cleaned/tree/staging) from master, merged it with cleaned_v0.6.0, and resolved some conflicts (conflicts were limited to the visualization files, which I manage, so I was confident in resolving them)
Merged changes from cleaned_dev_prs into staging without conflicts
Staging now contains all recent changes and is functioning smoothly

However, I noticed two issues:

1. Compilation Issue: The package fails to compile due to the input_mappings.R file (commited previously in the cleaned_dev_prs). I removed it in staging. It doesn't seem core to CLEANED, but if it's important, you may want to investigate the issue (remotes::install_github("CIAT/cleaned@cleaned_dev_prs")). Error looks to something like this: Error : cannot create dir 'data\mappings', reason 'No such file or directory' -- lexical error: invalid char in json text. data/qt_example.json

2. Nitrogen Balance Error: After merging the fertilizer issue fix, the previous error no longer occurs, but there's a new error message in the nitrogen_balance function, when the fertilizer is missing.

@M-Emmanuel
Copy link
Collaborator

Please ignore the input_mappings.R file, it is something under development and isn't linked to the CLEANED package at the moment. Could you please provide more details on the new error message in the nitrogen_balance function? I think this is what Stephen addressed last time. Happy to jump on a call.

Copy link
Collaborator

@M-Emmanuel M-Emmanuel left a comment

Choose a reason for hiding this comment

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

I would approve this given there's an upcoming training that will rely on these changes. Otherwise it would make sense to address the nitrogen_balance in the next commit. Hopefully you have tested this locally and everything seems to work except the above?

@ymutua
Copy link
Collaborator

ymutua commented Nov 20, 2024

Please ignore the input_mappings.R file, it is something under development and isn't linked to the CLEANED package at the moment. Could you please provide more details on the new error message in the nitrogen_balance function? I think this is what Stephen addressed last time. Happy to jump on a call.

How high can you jump @M-Emmanuel? Great to see the work going on here anyways!

@ahmedjoubest
Copy link
Collaborator Author

Could you please provide more details on the new error message in the nitrogen_balance function? I think this is what Stephen addressed last time. Happy to jump on a call.

Please find the error message below for reference.

Browse[1]> nitrogen_balance <- n_balance(
+           para = para,
+           land_required = land_required,
+           soil_erosion = soil_erosion
+         )
Error in para[["fertilizer"]][which(para[["fertilizer"]]$fertilizer_desc ==  : 
  incorrect number of dimensions

@M-Emmanuel
Copy link
Collaborator

Great

Could you please provide more details on the new error message in the nitrogen_balance function? I think this is what Stephen addressed last time. Happy to jump on a call.

Please find the error message below for reference.

Browse[1]> nitrogen_balance <- n_balance(
+           para = para,
+           land_required = land_required,
+           soil_erosion = soil_erosion
+         )
Error in para[["fertilizer"]][which(para[["fertilizer"]]$fertilizer_desc ==  : 
  incorrect number of dimensions

Great, I'll probe this.

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