-
Notifications
You must be signed in to change notification settings - Fork 19
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
DRAFT: Shabansky fix #24
Conversation
…umeing r==1.0. Fixed calculation of number of bounce regions -- it had a bug in it. Fixed search for shabansky I/2 FL -- it often failed due to bracket being too narrow.
…and ComputeI_FromMltMlat2() routines in order to return ErrorStatus.
… tracing routine. Added defaults for FluxtubeVolume tolerances. Added some missing memory management in Lgm_FreeMagInfo_children().
…s all still experimental.
… have multiple minima, I now do a pretrace and getm close to the global Bmin. This is then fed into the Lgm_TraceToMinBSurf() routine so that it converges on the true global Bmin. This fixes many issues with the Shabansky drift orbit stuff. NOTE HOWEVER: This really should be donem in Lgm_TraceToMinBSurf() in order to get consistentm results. (This does only change things for FLs with multiple miniman.)
…arther than we expected (returns -2 now instead of -1). Allows us to detect this error in the calling routines.
…hat should be made between Lgm_TraceToMinBSurf() and the Lgm_Trace() convenience routine.
…get ISearchMethod1 working at the same time.
Thanks @mghenderson64 - the Travis CI is reporting a failure to compile. It looks like Lgm_FluxTubeVolume.c is missing. If you can commit that to the same branch and push it'll automatically update the pull request and resubmit the tests. Once they've run I'll take a closer look. |
… to northern footpoints)
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.
A few questions/comments/requests, just working them as I go...
If I'm worrying about nothing on any of these, let me know.
- There's no
Makefile
for the FluxTubeVolume example. - The FluxTubeVolume example doesn't compile for me as it doesn't know where to look for the function definition. Is a header file missing?
- Travis CI is clearly mis-reporting the outcome of check. I get local failures of a number of tests. Can you run
make check
locally and inspect the output? My guess is that the algorithm changes would lead us to expect the observed regression test failures. If that's the case the expected results files should be updated. - Following the above, I think we still expect the weird Lm/L*/Lm failure, so don't worry about that test.
Examples/SimpleTrace
doesn't have aMakefile
Seems fine once compiled (but writes more output than I'd expect from an example program...)- Is it possible for the new
while
loop inBracketZero
to get stuck? Should there be a loop counter or some other check so it can't get stuck? - Why was the Jensen-Cain stuff removed from
Lgm_MagModelInfo_Set_MagModel
? If it was removed in error can you please put it back (inLgm_InitMagInfo.c
)? - Line 234 of
Lgm_Trace.c
has a command hidden behind a check for a verbosity setting of -100!! Is this a debug that you're hiding? That you meant to remove? I guess it doesn't really hurt for it to be there, but it's never going to appear with any standard verbosity settings. - The section in
Lgm_Trace
(used to be line 258) removes a check for the return type inLgm_TraceToMinBSurf
. The changes toLgm_TraceToMinBSurf.c
only add comments about fixing the behaviour that would require this check - the behaviour isn't actually fixed yet. So shouldn't we be keeping this check?
I haven't pushed any of this yet, but working on it....
* There's no Makefile for the FluxTubeVolume example.
Added
* The FluxTubeVolume example doesn't compile for me as it doesn't know where to look for the function definition. Is a header file missing?
changed name in .h file to the correct name (had FluxTubeVolume instead of Lgm_FluxTubeVolume)
* Travis CI is clearly mis-reporting the outcome of check. I get local failures of a number of tests. Can you run make check locally and inspect the output? My guess is that the algorithm changes would lead us to expect the observed regression test failures. If that's the case the expected results files should be updated.
Looking at these now. Results are close. Possibly IGRF changes or tolerances in BS.
* Following the above, I think we still expect the weird Lm/L*/Lm failure, so don't worry about that test.
I figured out what at least a big part of this is due to. I will add those fixes as well...
* Examples/SimpleTrace doesn't have a Makefile Seems fine once compiled (but writes more output than I'd expect from an example program...)
Added.
* Is it possible for the new while loop in BracketZero to get stuck? Should there be a loop counter or some other check so it can't get stuck?
I will check this.
* Why was the Jensen-Cain stuff removed from Lgm_MagModelInfo_Set_MagModel? If it was removed in error can you please put it back (in Lgm_InitMagInfo.c)?
Unintentional, I will add back.
* Line 234 of Lgm_Trace.c has a command hidden behind a check for a verbosity setting of -100!! Is this a debug that you're hiding? That you meant to remove? I guess it doesn't really hurt for it to be there, but it's never going to appear with any standard verbosity settings.
Haha. I'll nuke that -- was just for debugging.
* The section in Lgm_Trace (used to be line 258) removes a check for the return type in Lgm_TraceToMinBSurf. The changes to Lgm_TraceToMinBSurf.c only add comments about fixing the behaviour that would require this check - the behaviour isn't actually fixed yet. So shouldn't we be keeping this check
Yup. I'll add that back.
…________________________________
From: Steve Morley <notifications@github.com>
Sent: Thursday, September 19, 2019 3:19:56 PM
To: drsteve/LANLGeoMag
Cc: Henderson, Michael Gerard; Mention
Subject: Re: [drsteve/LANLGeoMag] Shabansky fix (#24)
@drsteve requested changes on this pull request.
A few questions/comments/requests, just working them as I go...
If I'm worrying about nothing on any of these, let me know.
* There's no Makefile for the FluxTubeVolume example.
* The FluxTubeVolume example doesn't compile for me as it doesn't know where to look for the function definition. Is a header file missing?
* Travis CI is clearly mis-reporting the outcome of check. I get local failures of a number of tests. Can you run make check locally and inspect the output? My guess is that the algorithm changes would lead us to expect the observed regression test failures. If that's the case the expected results files should be updated.
* Following the above, I think we still expect the weird Lm/L*/Lm failure, so don't worry about that test.
* Examples/SimpleTrace doesn't have a Makefile Seems fine once compiled (but writes more output than I'd expect from an example program...)
* Is it possible for the new while loop in BracketZero to get stuck? Should there be a loop counter or some other check so it can't get stuck?
* Why was the Jensen-Cain stuff removed from Lgm_MagModelInfo_Set_MagModel? If it was removed in error can you please put it back (in Lgm_InitMagInfo.c)?
* Line 234 of Lgm_Trace.c has a command hidden behind a check for a verbosity setting of -100!! Is this a debug that you're hiding? That you meant to remove? I guess it doesn't really hurt for it to be there, but it's never going to appear with any standard verbosity settings.
* The section in Lgm_Trace (used to be line 258) removes a check for the return type in Lgm_TraceToMinBSurf. The changes to Lgm_TraceToMinBSurf.c only add comments about fixing the behaviour that would require this check - the behaviour isn't actually fixed yet. So shouldn't we be keeping this check?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#24?email_source=notifications&email_token=ANFHIQKBNVQIUIE6FGSXZGTQKPUHZA5CNFSM4IYMU4CKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCFLAIFI#pullrequestreview-290849813>, or mute the thread<https://github.com/notifications/unsubscribe-auth/ANFHIQM5VL5ZTFUNFAYTUYTQKPUHZANCNFSM4IYMU4CA>.
|
Hey @mghenderson64 - any luck updating this? If you have issues with conflicts, etc. that are causing trouble, let me know and I can try to fix things. |
b024da6
to
de532b9
Compare
Changes include: