-
Notifications
You must be signed in to change notification settings - Fork 156
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
GitHub Issue NOAA-EMC/GSI#310 and #367. Correct RadMon's mean correction time series plots and update html generation. #366
Conversation
Hi @EdwardSafford-NOAA. This PR has four commits. To reduce the number of commits:
This will reduce the four commits to a single commit. This change should be merged to the authoritative develop branch next Tuesday (when the last of the GFS v16.x updates will be merged to develop). If you have any questions, please let me know. |
@MichaelLueken-NOAA I must have messed something up. Now I have 5 commits instead of 4. My guess is I messed up the transition from master to develop. I did have problems quite a bit of a struggle with that, but thought I had it worked out. |
@EdwardSafford-NOAA. What types of issues did you encounter while going through the previous steps? It looks like you might have used
At this point, I have:
Please don't use the recommended git push origin develop --force to push the change back to the repository. |
ba49eae
to
622e07c
Compare
@MichaelLueken-NOAA Thanks for your continued patience. I think it's ok now. |
@EdwardSafford-NOAA Yep, everything looks good. I should be able to merge this next Tuesday after the current work out to the review committee is merged. Thank you very much for doing this! |
622e07c
to
7f41bb4
Compare
@MichaelLueken-NOAA I have to offer apologies again for my incompetence. I squash merged my 2nd dev branch, RadMon_367, into my develop branch and committed it with --amend and --no-edit, but I still managed to add 4 commits instead of just the one. I have no idea how I messed up but I sure did. Sorry for creating more work for you. I'll be glad to fix it if you tell me how. |
@EdwardSafford-NOAA No worries. Please try the following:
These steps should allow you clone get both issue #310 and #367 changes into your fork's develop branch with only a single commit message. |
7f41bb4
to
b7631a3
Compare
@MichaelLueken-NOAA thanks as always for your help. I think it's ok now. |
@EdwardSafford-NOAA Yep, everything looks good now. I have also added issue #367 to the list of issues that can be closed with this PR. Is there anyone you would like to have review the html changes, or should that not be required? |
@MichaelLueken-NOAA a review isn't essential; these changes are in the non-operational portion of the package and are not routinely used. If @DavidHuber-NOAA is free I'd welcome his $0.02, but I wouldn't hold up release if he's not able to look it over. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Thanks!
@EdwardSafford-NOAA Please use the following commands to reduce the 2 commits to a single commit:
Thanks! |
43c54de
to
f16e852
Compare
@MichaelLueken-NOAA sorry, I got ahead of things and updated my develop branch from upstream, forgetting that would mess up this PR. |
Since there are no source code changes and @DavidHuber-NOAA has reviewed the changes, will now give final approval to these changes and merge them to the authoritative repository. |
Why was a tarball |
The pngs are used by the RadMon's website building script on the splash page. I guess the thumbs could be left on the server and manipulated remotely, but it made sense to me to put all the RadMon html related files in one place. Also, from what I've heard there is no backup for emcrzdm, so a copy might be useful. |
Generally speaking, committing binary files to a git repository is not recommended. NOAA-EMC does not pay for Git-LFS (a solution for storing binary files in Git on GitHub). Having said that, if the thumbs will never be updated (.tar), then I think this will likely not be a concern. |
@aerorahul Thanks for the detailed explanation. I wasn't aware of the git issues with binary files. Yes, your sense is right -- the thumbs will change over time in response to changes in available instruments. I'll make a point of replacing the file as needed rather than updating it. |
@EdwardSafford-NOAA replacing the file is the same as adding new file. The history will still contain the old file and a new file. I don't think that changes anything.
and then you add
then I think we can live with it. But, if you replace |
@aerorahul The individual thumbs will never be updated -- there isn't any need for that. I'll open a new issue to remove the tar and move the thumbs to a new png directory. |
@EdwardSafford-NOAA |
@aerorahul Sorry, I had no idea the scope of this problem. I'll investigate how to remove the binaries from history. |
@aerorahul I read there are some tools that will assist with rewriting history but I assume they are not available. I tested this solution locally:
That seems to work -- the file is removed and the last 5 commits are modified to remove all references to the tar file. I assume that @MichaelLueken-NOAA would need to run this on the authoritative develop branch in order to really implement it. As for branches created from develop since the merger (f16e852), I'm not yet sure how best to handle that. |
@EdwardSafford-NOAA I think any branch created since the merge will have to do the same on the branch on the users fork. I am not sure about this next bit, but I think the But, as you can see, once a binary file gets in, it gets very difficult to get rid of its traces, especially when there is a large community involved. |
That is very clear now. I am really sorry my ignorance resulted in this mess. I explored interactive rebase as well. That's a bit less invasive, modifying only the f16e852 commit, but from what I read, it doesn't remove the associated history. So I don't think that's the best option, though it looks like a useful thing to know about for use in less dire circumstances. |
@aerorahul @MichaelLueken-NOAA I've continued to investigate the options to remove the binary file. From what I read, after the filter command on the authoritative develop branch, a push to the develop branch on user forks should be sufficient. None of the sources I've read suggest re-forking is necessary. That said, I can't rule that possibility out. If the decision is that we can live with the current situation, then I will not make any changes to the binary, ever. If the filter process is deemed too cumbersome/risky, then I think I should go ahead and remove the binary (knowing that doesn't solve the problem, but it does prevent any change to teh binary and thus additional growth in the size of the repo). I can adjust the install scripts to work around that and store the binary on the server (I have a local copy as well). Thoughts? |
@EdwardSafford-NOAA Is there a way for @MichaelLueken-NOAA to verify (for the next few PR's that come in), to ensure that they do not contain this binary file in their history. This is just to ensure that the users did infact get the push to their |
@aerorahul Indeed there is. Running:
Returns this: I cloned a new GSI repo, created a new branch, When I run the git log command (above) on branch As a sanity check I then manually removed the file from The git log command on So I think running that git log command on candidate merger branches will do the trick. |
@aerorahul @EdwardSafford-NOAA I have submitted an update to the review committee, so no changes will be made to the authoritative repository until that work has been approved and merged (next Wednesday). Once the work has been merged, I will use the:
command (10 rather than 8 due to the update to the develop branch this morning and the IR bug fix update middle of next week), and then let developers know to use the command in their forks so that the tar file and history are removed. Thanks! |
@MichaelLueken-NOAA I'm good with that schedule. I'd like to do some experimentation as part of the filter, provided you're OK with it. I want to set up two different clones before you run the filter command. After you run the filter I'll do a force pull on one and run the same filter command on the other and compare results. I'm just curious to see if both strategies work as I expect. If they do then users will have some options. Also I assume there's some sort of backup in place for the authoritative |
@EdwardSafford-NOAA Yes, I don't do anything to the authoritative repository without having the authoritative develop branch cloned in a static location first (the control used for the regression testing). So, not to worry there. If there is an issue, I'd just force push this control local copy that doesn't include the filter command. I'll let you know once PR #374 has been merged so that you can prep your experiment. Once you give me the green light, I'll disseminate the filter command (or whatever the best path forward is, once it has been decided) to the users of the NOAA-EMC/GSI repository. |
@MichaelLueken-NOAA I'm set up to run my experiments. Just let me know when you've run the filter command on the upstream develop branch and I will proceed. |
@EdwardSafford-NOAA I have three updates to merge to the authoritative repository today, then I will move forward with the filter command. I'll let you know through this PR when the filter command has been run. |
@EdwardSafford-NOAA I have force pushed the filter command now. Please let me know once your experiment is complete so that I can let the developers know what they need to do to bring in the change. |
@MichaelLueken-NOAA here's what I did. I'll be glad to answer any questions. Again, thanks for your patience and, while I regret making the mistake, I have learned a great deal about git as a result.
Summation: I'm disappointed but not surprised to learn that a force pull from upstream/develop is insufficient to overwrite the local history. But it looks like running the filter command on user's develop branch (followed by the log command to make sure the filter reached back far enough) will fix the problem. I didn't try re-forking but I'm sure that would work too. |
GitHub Issue NOAA-EMC#310 and NOAA-EMC#367. Correct RadMon's mean correction time series plots and update html generation.
GitHub Issue NOAA-EMC#310 and NOAA-EMC#367. Correct RadMon's mean correction time series plots and update html generation.
commit 112ca82 Merge: 257f6eb dd1bac5 Author: RussTreadon-NOAA <26926959+RussTreadon-NOAA@users.noreply.github.com> Date: Tue Jun 21 09:56:10 2022 -0400 Merge branch 'NOAA-EMC:develop' into feature/rm_sfcanl commit 257f6eb Merge: 9c48261 ad84f17 Author: RussTreadon-NOAA <russ.treadon@noaa.gov> Date: Mon Jun 13 14:52:28 2022 +0000 Merge branch 'develop' into feature/rm_sfcanl commit 9c48261 Merge: dc62da7 eb220ca Author: RussTreadon-NOAA <26926959+RussTreadon-NOAA@users.noreply.github.com> Date: Thu May 26 05:43:20 2022 -0400 Merge branch 'NOAA-EMC:develop' into feature/rm_sfcanl commit eb220ca Author: michael.lueken <Michael.Lueken@noaa.gov> Date: Wed May 25 16:04:25 2022 +0000 GitHub Issue NOAA-EMC#382. Add GSI fix file changes for v16.3 commit 8c22b33 Merge: a90b632 cf4b323 Author: MichaelLueken-NOAA <63728921+MichaelLueken-NOAA@users.noreply.github.com> Date: Wed May 25 15:56:49 2022 +0000 Merge pull request NOAA-EMC#374 from ADCollard/IRBugFix GitHub Issue NOAA-EMC#371. Minor bug fixes to IR processing commit dc62da7 Merge: b7afecf a90b632 Author: RussTreadon-NOAA <26926959+RussTreadon-NOAA@users.noreply.github.com> Date: Fri May 20 12:10:17 2022 -0400 Merge branch 'NOAA-EMC:develop' into feature/rm_sfcanl commit a90b632 Merge: 524527b 889cce5 Author: MichaelLueken-NOAA <63728921+MichaelLueken-NOAA@users.noreply.github.com> Date: Fri May 20 16:08:11 2022 +0000 Merge pull request NOAA-EMC#386 from HaixiaLiu-NOAA/master GitHub Issue NOAA-EMC#385. Add seviri_m09 to Radiance_Monitor util commit 889cce5 Author: Haixia.Liu <Haixia.Liu@noaa.gov> Date: Fri May 20 04:27:21 2022 +0000 GitHub Issue NOAA-EMC#385. Add seviri_m09 to Radiance_Monitor util commit b7afecf Merge: 58b52c6 524527b Author: RussTreadon-NOAA <26926959+RussTreadon-NOAA@users.noreply.github.com> Date: Thu May 19 12:46:19 2022 -0400 Merge branch 'NOAA-EMC:develop' into feature/rm_sfcanl commit 524527b Merge: 9946c69 002dc87 Author: MichaelLueken-NOAA <63728921+MichaelLueken-NOAA@users.noreply.github.com> Date: Thu May 19 16:08:48 2022 +0000 Merge pull request NOAA-EMC#379 from AndrewEichmann-NOAA/EXP-efso_fv3_issue_378 Github issue NOAA-EMC#378: refactoring of EFSOI scripts and compiled code (EFSOI and EFSOI forecast scripts) commit 9946c69 Merge: bcc0358 ccfda16 Author: MichaelLueken-NOAA <63728921+MichaelLueken-NOAA@users.noreply.github.com> Date: Thu May 19 16:05:20 2022 +0000 Merge pull request NOAA-EMC#381 from emilyhcliu/feature/gmi GitHub Issue NOAA-EMC#341. Add GMI for radmon commit bcc0358 Merge: 7146a0b 7a4fa9b Author: MichaelLueken-NOAA <63728921+MichaelLueken-NOAA@users.noreply.github.com> Date: Thu May 19 16:02:55 2022 +0000 Merge pull request NOAA-EMC#384 from christopherwharrop-noaa/feature/update-cheyenne-hpc-stack GitHub Issue NOAA-EMC#383 Update hpc-stack in Cheyenne modulefiles for GNU and Intel compilers. commit 7a4fa9b Author: Christopher Harrop <Christopher.W.Harrop@noaa.gov> Date: Thu May 19 09:23:33 2022 -0600 GitHub Issue NOAA-EMC#383 Update hpc-stack in Cheyenne modulefiles for GNU and Intel compilers. A small workaround was necessary for the GNU modulefile to avoid a dependency issue with the GNU version of hpc-stack. commit ccfda16 Author: Emily Liu <emily.liu@noaa.gov> Date: Thu May 19 02:20:09 2022 +0000 Add GMI for radmon commit 58b52c6 Merge: c4c860b 7146a0b Author: RussTreadon-NOAA <26926959+RussTreadon-NOAA@users.noreply.github.com> Date: Wed May 18 14:52:48 2022 -0400 Merge branch 'NOAA-EMC:develop' into feature/rm_sfcanl commit 7146a0b Author: Rahul Mahajan <aerorahul@users.noreply.github.com> Date: Wed May 18 11:57:08 2022 -0400 GitHub Issue NOAA-EMC#112. A refactor of CMake build framework. (NOAA-EMC#327) commit cf4b323 Author: Andrew Collard <andrew.collard@noaa.gov> Date: Fri Apr 29 21:34:22 2022 +0000 GitHub Issue NOAA-EMC#371: Bug fixes to IR processing commit 002dc87 Author: AndrewEichmann-NOAA <Andrew.Eichmann@noaa.gov> Date: Thu May 12 15:02:40 2022 -0500 Removed extraneous lines from EFSOI script, and extraneous exgdas_efsoi_fcst.sh script commit c4c860b Merge: c9ac824 34294c1 Author: RussTreadon-NOAA <russ.treadon@noaa.gov> Date: Mon May 9 16:46:28 2022 +0000 Merge branch 'develop' into feature/rm_sfcanl commit 34294c1 Merge: ad2a3c0 20cf887 Author: MichaelLueken-NOAA <63728921+MichaelLueken-NOAA@users.noreply.github.com> Date: Mon May 9 16:26:12 2022 +0000 Merge pull request NOAA-EMC#376 from EdwardSafford-NOAA/develop GitHub Issue NOAA-EMC#373. Compress RadMon data files. commit ad2a3c0 Merge: e7f8c88 845b360 Author: MichaelLueken-NOAA <63728921+MichaelLueken-NOAA@users.noreply.github.com> Date: Mon May 9 16:24:29 2022 +0000 Merge pull request NOAA-EMC#369 from MichaelLueken-NOAA/feature/wcoss2_port GitHub Issue NOAA-EMC#368. WCOSS2 port. commit 20cf887 Author: Edward.Safford <edward.safford@noaa.gov> Date: Thu May 5 12:04:30 2022 +0000 Github issue NOAA-EMC#373. Compress RadMon data files. Completes NOAA-EMC#373. commit 845b360 Author: Michael Lueken <michael.lueken@noaa.gov> Date: Fri Apr 29 11:00:52 2022 +0000 GitHub Issue NOAA-EMC#368. WCOSS2 port. commit c9ac824 Merge: e577941 e7f8c88 Author: RussTreadon-NOAA <26926959+RussTreadon-NOAA@users.noreply.github.com> Date: Tue May 3 10:42:43 2022 -0400 Merge branch 'NOAA-EMC:develop' into feature/rm_sfcanl commit e7f8c88 Merge: 586c2df 0a5daea Author: MichaelLueken-NOAA <63728921+MichaelLueken-NOAA@users.noreply.github.com> Date: Tue May 3 10:08:34 2022 -0400 Merge pull request NOAA-EMC#372 from RussTreadon-NOAA/feature/interp GitHub Issue NOAA-EMC#370: use nf90_inq_varid to set integer addresses for lon and lat variables commit 0a5daea Author: RussTreadon-NOAA <russ.treadon@noaa.gov> Date: Fri Apr 29 21:36:35 2022 +0000 GitHub Issue NOAA-EMC/GSI NOAA-EMC#370: use nf90_inq_varid to set integer addresses for lon and lat variables commit e577941 Merge: be550bf 586c2df Author: RussTreadon-NOAA <26926959+RussTreadon-NOAA@users.noreply.github.com> Date: Tue May 3 09:52:08 2022 -0400 Merge branch 'NOAA-EMC:develop' into feature/rm_sfcanl commit 586c2df Merge: 87fe77d f16e852 Author: MichaelLueken-NOAA <63728921+MichaelLueken-NOAA@users.noreply.github.com> Date: Tue May 3 09:46:33 2022 -0400 Merge pull request NOAA-EMC#366 from EdwardSafford-NOAA/develop GitHub Issue NOAA-EMC#310 and NOAA-EMC#367. Correct RadMon's mean correction time series plots and update html generation. commit f16e852 Author: edward.safford <edward.safford@noaa.gov> Date: Fri Apr 29 14:39:22 2022 +0000 Github issue NOAA-EMC#310. Correct mean correction time series plots. Fixes NOAA-EMC#310. commit be550bf Merge: c0a5204 114ed31 Author: RussTreadon-NOAA <26926959+RussTreadon-NOAA@users.noreply.github.com> Date: Mon May 2 06:25:23 2022 -0400 Merge branch 'NOAA-EMC:develop' into feature/rm_sfcanl commit c0a5204 Merge: 8b376d1 498896b Author: RussTreadon-NOAA <26926959+RussTreadon-NOAA@users.noreply.github.com> Date: Mon Apr 25 13:22:30 2022 -0400 Merge branch 'NOAA-EMC:develop' into feature/rm_sfcanl commit 8b376d1 Merge: 91a8077 0c6548e Author: RussTreadon-NOAA <26926959+RussTreadon-NOAA@users.noreply.github.com> Date: Wed Apr 20 13:53:51 2022 -0400 Merge branch 'NOAA-EMC:master' into feature/rm_sfcanl commit 91a8077 Merge: dec3299 7201778 Author: RussTreadon-NOAA <26926959+RussTreadon-NOAA@users.noreply.github.com> Date: Wed Apr 13 10:27:36 2022 -0400 Merge branch 'NOAA-EMC:master' into feature/rm_sfcanl commit dec3299 Merge: 6204f98 5936a23 Author: RussTreadon-NOAA <26926959+RussTreadon-NOAA@users.noreply.github.com> Date: Tue Apr 5 10:49:24 2022 -0400 Merge branch 'NOAA-EMC:master' into feature/rm_sfcanl commit 6204f98 Merge: f7a1495 affe4ed Author: RussTreadon-NOAA <26926959+RussTreadon-NOAA@users.noreply.github.com> Date: Thu Mar 31 12:38:40 2022 -0400 Merge branch 'NOAA-EMC:master' into feature/rm_sfcanl commit f7a1495 Merge: b592666 ef237ce Author: RussTreadon-NOAA <26926959+RussTreadon-NOAA@users.noreply.github.com> Date: Thu Mar 24 08:57:46 2022 -0400 Merge branch 'NOAA-EMC:master' into feature/rm_sfcanl commit b592666 Merge: d0f7c20 47ee70b Author: RussTreadon-NOAA <26926959+RussTreadon-NOAA@users.noreply.github.com> Date: Thu Mar 10 09:13:14 2022 -0500 Merge branch 'NOAA-EMC:master' into feature/rm_sfcanl commit d0f7c20 Merge: ed94193 828c0cf Author: RussTreadon-NOAA <26926959+RussTreadon-NOAA@users.noreply.github.com> Date: Wed Mar 2 10:35:21 2022 -0500 Merge branch 'NOAA-EMC:master' into feature/rm_sfcanl commit ed94193 Merge: c63bd77 963b7e5 Author: RussTreadon-NOAA <russ.treadon@noaa.gov> Date: Tue Mar 1 16:04:48 2022 +0000 Merge branch 'master' at 963b7e5 into feature/rm_sfcanl Conflicts: scripts/exglobal_atmos_analysis.sh commit c63bd77 Merge: 39912e1 0444258 Author: RussTreadon-NOAA <26926959+RussTreadon-NOAA@users.noreply.github.com> Date: Tue Feb 15 06:06:10 2022 -0500 Merge branch 'NOAA-EMC:master' into feature/rm_sfcanl commit 39912e1 Merge: f0161c8 9e765da Author: RussTreadon-NOAA <26926959+RussTreadon-NOAA@users.noreply.github.com> Date: Tue Jan 11 06:40:58 2022 -0500 Merge branch 'NOAA-EMC:master' into feature/rm_sfcanl commit f0161c8 Merge: a75d6ed a62dec6 Author: RussTreadon-NOAA <26926959+RussTreadon-NOAA@users.noreply.github.com> Date: Tue Dec 14 11:40:31 2021 -0500 Merge branch 'NOAA-EMC:master' into feature/rm_sfcanl commit a75d6ed Author: russ.treadon <Russ.Treadon@noaa.gov> Date: Tue Dec 14 14:38:57 2021 +0000 GitHub Issue NOAA-EMC#266: add DOGAUSFCANL capability back to JGLOBAL_ATMOS_ANALYSIS_CALC and exglobal_atmos_analysis_calc.sh commit 9422758 Author: russ.treadon <Russ.Treadon@noaa.gov> Date: Mon Dec 13 14:39:14 2021 +0000 GitHub Issue NOAA-EMC#266: remove DOGCYCLE and DOGAUSFCANL scripting and variables from j-jobs and scripts * jobs/JGDAS_ATMOS_ANALYSIS_DIAG - remove DOGCYCLE and DOGAUSFCANL * jobs/JGLOBAL_ATMOS_ANALYSIS - remove DOGCYCLE and DOGAUSFCANL * jobs/JGLOBAL_ATMOS_ANALYSIS_CALC - remove DOGCYCLE and DOGAUSFCANL * scripts/exglobal_atmos_analysis.sh - remove DOGCYCLE scripting and DOGAUSFCANL variables * scripts/exglobal_atmos_analysis_calc.sh - remove DOGAUSFCANL scripting
Fix the mean correction plots in the RadMon, which often displayed all 0 values for the sdv.
This issue was traced back to the numerical format used in the files sent to the server. The format did not include enough digits beyond the decimal point and often resulted in rounding to 0.
Note that this improves the situation but more work needs to be done. In the resulting plots the mouse-over values in the sdv plots, even when indicated they are non-zero, are still rounding to 0 in the mouse-over value display. It may be possible to improve the javascript formatting of the data values. It also may be that reverting to static plots might be a more satisfactory solution, though the ability to compare values on the fly would be lost.
@MichaelLueken-NOAA this change is trivial enough that I don't think a code review, beyond your once-over, is necessary.