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

add real-scorecard to MONET #227

Merged
merged 18 commits into from
Jun 24, 2024
Merged

Conversation

btang1
Copy link
Contributor

@btang1 btang1 commented Jan 11, 2024

add real-score card to MONET

note #219 should be merged before this

Copy link
Collaborator

@rschwant rschwant left a comment

Choose a reason for hiding this comment

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

The real-scorecard updates and description look good here. My concern is it looks like the multi-boxplot stuff is also in this pull request. But this already got merged to develop and I think is what is causing the merge conflicts in this pull request. @zmoon do you have suggestions to Beiming on how to fix this within GitHub, so this pull request only updates the scorecard components?

@zmoon
Copy link
Collaborator

zmoon commented May 2, 2024

@rschwant this has to be fixed locally on the command line (by merging the upstream develop branch into @btang1 's branch), @btang1 has some experience resolving merge conflicts this way already.

@rschwant
Copy link
Collaborator

@btang1 you mentioned last week that this was ready, but it looks like there are still merge conflicts? I can review this Thurs/Fri next week. Just let me know when it is ready.

@btang1
Copy link
Contributor Author

btang1 commented May 19, 2024

@btang1 you mentioned last week that this was ready, but it looks like there are still merge conflicts? I can review this Thurs/Fri next week. Just let me know when it is ready.

Thanks will address the merge problem!

Conflicts:
	docs/appendix/yaml.rst
	docs/examples/airnow_wrfchem.ipynb
	docs/examples/control_wrfchem_mech-0905_2.yaml
	melodies_monet/driver.py
	melodies_monet/plots/surfplots.py
@btang1
Copy link
Contributor Author

btang1 commented May 21, 2024

@rschwant Hi Becky, @zmoon Helped me fix the merge bug. now we are good to merge. could you please take a look?

Copy link
Collaborator

@rschwant rschwant left a comment

Choose a reason for hiding this comment

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

Looks good!

@rschwant
Copy link
Collaborator

@btang1, this looks good. Although GitHub still says there are some merge conflicts that need to be resolved before merging. Can you update these and then I can merge this in? I don't see any more problems with the multi-boxplot, so maybe these are other problems?

@rschwant
Copy link
Collaborator

Maybe this is because I merged in your other pull request. And this caused some conflicts with this code now?

@btang1
Copy link
Contributor Author

btang1 commented May 24, 2024

Maybe this is because I merged in your other pull request. And this caused some conflicts with this code now?

@rschwant Yes! the scorecard has incorporate most multi-box plot code. but not CSI code yet. So once you merge CSI code. The scorecard code has merging problem again. no worries. I will try to fix it~

…card

Conflicts:
	docs/appendix/yaml.rst
	docs/examples/airnow_wrfchem.ipynb
	melodies_monet/driver.py
	melodies_monet/plots/surfplots.py
@btang1
Copy link
Contributor Author

btang1 commented Jun 9, 2024

@rschwant Hi Becky, I have merged changes from CSI to scorecard. and test it example output in: https://github.com/btang1/MELODIES-MONET/blob/real-scorecard/docs/examples/airnow_wrfchem.ipynb.
now it should be ready to merge. could you please let me know if it works?

@rschwant
Copy link
Collaborator

@btang1 Just to add this here too. Can you fix the error in this file? https://github.com/btang1/MELODIES-MONET/blob/real-scorecard/docs/examples/airnow_wrfchem.ipynb. It looks like for the csi plot, you do not define a region_bx, so it shouldn't be listed on line 1989 to delete? Probably when you did the merge fix this did not merge perfectly. So I think you need to delete "region_bx" from this line and re-run.

@rschwant
Copy link
Collaborator

This works now thanks @zmoon and @btang1! I'm merging this in now, so that NCAR's examples work properly.

@rschwant rschwant merged commit c618652 into NOAA-CSL:develop Jun 24, 2024
4 checks passed
@btang1
Copy link
Contributor Author

btang1 commented Jun 24, 2024

@rschwant @zmoon thank you so much!!!!

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