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

Fixed -1 error in cnvmodel.c #623

Merged
merged 2 commits into from
Dec 2, 2024
Merged

Conversation

RemingtonRohel
Copy link

Reverting error introduced with #620.

Tested by using map_addmodel -old_aacgm from the commit before #620 was merged and verifying identical fields in the resulting map file. I used dmap to read in the files, then used straight equality checks for each field of each record.

@egthomas
Copy link
Member

egthomas commented Oct 8, 2024

@RemingtonRohel thanks for following up on this, I really appreciate it. I'll try to test this shortly.

@RemingtonRohel
Copy link
Author

@egthomas I'm sorry you even had to ask, I should've caught this before making my original PR. If you find any issues with the binaries I'll fix them up as quick as I can.

@aburrell
Copy link
Contributor

@RemingtonRohel this is on me, too, as code reviewer. I wish we had a unit test suite to catch these problems!

@egthomas
Copy link
Member

@RemingtonRohel I've just had a chance to test this, and am still seeing some differences. To test, I've used this branch and commit 577bfbc.

First, I tried solve_model on both versions and received slightly different output, eg

solve_model > out.txt
vi out.txt

Next, I tried creating an empty map file with just model vectors and plotting it, eg

map_grd -sd 20241128 -ex 01:00 -empty > empty.grd.cnvmap
map_addhmb -nodef empty.grd.cnvmap > empty.hmb.cnvmap
map_addmodel -o 8 empty.hmb.cnvmap > empty.model.cnvmap
map_fit empty.model.cnvmap >  empty.fit.cnvmap
map_plot -x -def -mod empty.fit.cnvmap

and the contours and cross-polar cap potential appear to be slightly different, eg first from this branch

test

and then with the pre-#620 commit

test

Are you able to replicate these differences? It's possible I may have mixed something up, but it would be helpful to confirm. Thanks!

@RemingtonRohel
Copy link
Author

Hi Evan, thanks for testing this.

I was not able to reproduce both plots; using this branch and commit 577bfbc both yielded the second plot for me. I copied your commands verbatim.

I did have a little hiccup when testing where I forgot to rebuild RST, and didn't redirect my PATH to ensure that solve_model and other RST commands were actually using the correct scripts (i.e. those in the directory of my RST fork). Is it possible one of these issues occurred for you? If not, I'm not sure where the differences in our testing might have come from.

To be verbose, my testing was:

  1. Checkout the appropriate commit (577bfbc or 7bc12fd3)
  2. make.build && make.code
  3. run which solve_model to make sure that the correct binary is being used (I have both an rst clone and an rst-rem clone)
  4. Run the commands you sent and view the plot

@RemingtonRohel
Copy link
Author

I did also find one minor typo in the fitacf 2.5 changes I made, just pushed that now.

@egthomas
Copy link
Member

egthomas commented Dec 2, 2024

@RemingtonRohel sorry - I must have made a mistake somewhere when trying to copy over your changes. I've just tested again with a fresh clone of this branch and the develop branch from your repository, and I can confirm the model output now matches. Thanks for checking again, and for catching the other fix in FITACF 2.5!

@egthomas egthomas merged commit cf8a218 into SuperDARN:develop Dec 2, 2024
1 of 3 checks passed
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