#121 Merged
  1. dcollins4096

Here's the PR for mhdct.

Comments (19)

  1. dcollins4096 author

    I've updated this PR to reflect comments and changes. This includes removal of the #ifdef MHDCT, printf syntax, and a re-write of the mhd_li.F method to accommodate arbitrary number of colour fields.

    1. MattT

      I would like to request to reviewers that further comments be made here in the pull request.

  2. Nathan Goldbaum

    Hi Dave,

    For what it's worth, I don't see the failures on the PhotonShadowing test you pointed out. I ran the answer tests using a locally generated gold standard from the current tip of enzo-dev, including strict tolerances and bitwise tests. My test results are pasted here: http://paste.yt-project.org/show/3362/

    I do see failures for the Orszag Tang test - but seeing how you updated that test below, I suspect that is the expected behavior and that the gold standard will have to be regenerated once this is accepted.

    1. dcollins4096 author

      Not in PhotonShadowing, but PhotonTestAMR. But I'm guessing you aren't seeing that, either.

      The result of OrszagTang has changed with the updated value of Pi. The new version is better.

      Thanks for testing, I appreciate it!

      1. Nathan Goldbaum

        Hey Dave,

        So I finally ran the full push suite for MHDCT using a locally generated gold standard, strict relative tolerances, and tests for bitwise identicality.

        In addition to the failure you noted for PhotonTestAMR (which I also see on the enzo-3.0 fork) I also see failures for NohProblem2DAMR as well as the two new Orszag Tang tests you've added. Of course since the Orszag Tang problems are not included in the current answer testing suite, I'm not surprised about these failures. The test results are pasted here:


        I've also pasted my compiler, version, config information, and compile time flags summary here:





        The differences in NohProblem2DAMR are small but a bit worrisome. For comparison I am able to get bitwise identical results compared to my local gold standard using my enzo-3.0 PR. Please let me know if you need any more information to reproduce this behavior on your end.

        Hope that's useful to you :)

        1. dcollins4096 author

          That's definitely useful, thanks! I didn't see problems with Noh this time around, but I find that one to be kind of touchy. I'll look into it.

  3. Greg Bryan

    Dave -- I just wanted to add two things: (i) this is fabulous work! I've been spending some time looking at this while working on the method paper and I'm really excited about getting it merged in. (ii) Matt and I have been working on putting the new Field object into this, to cut down on some of the new code, particularly in Grid_CommunicationSendRegion and Grid_CommunicationReceiveRegion. Hopefully we'll be ready to bring that in the next week or two (but if it's more than that, then this shouldn't hold up acceptance). Thanks for the great work!

    1. Brian OShea


      This sounds like a good plan regarding the Field object project. Could we possible set a firm deadline for getting this code into Enzo and tested, so the MHDCT pull request doesn't hang in limbo? It's April 17th today... how about May 1st?

      1. Greg Bryan

        Yes, May 1st sounds good. Matt and I just looked at this today, and the path seems clear.

        1. Brian OShea

          Hi guys,

          I just wanted to follow up, since May 1st is tomorrow. Is the Field object ready to go, or should we go ahead and accept this PR, then pull in the Field object later?


          1. MattT

            Hi Brian,

            Greg and I have discussed it and as a result of circumstances beyond our control, we have been unable to make this deadline. We're going to hold off on making changes or further requests of this pull request. However, as a compromise, we will be pushing on field objects in the "enzo-dev" branch (which is distinct from the enzo-dev repository), and request that the MHDCT be held back from enzo-dev until we have finished that work, so that the MHDCT changes can be integrate with those. We will handle that pull request and modifications to the code to ensure that happens.

  4. dcollins4096 author

    I spoke with Nathan yesterday, and we came to the conclusion that the Noh2DAMR failure was due to an inconsequential accumulation of errors.

    I just merged in from enzo-dev and updated the PR.

    I'm ready if you are.

  5. Sam Skillman

    I have just run the test_runner on strict for the push suite on 74babf3f0472 against enzogold0002, and it produces bitwise identical results for all but one test: Noh2DAMR that all look like:

    NohProblem2DAMR__test_noh2damr.test_noh2damr: FAILURE (<type 'exceptions.AssertionError'>, AssertionError('
        Not equal to tolerance rtol=1e-13, atol=0
        (mismatch 100.0%)
         x: array([  7.6608567 ,   6.09418351,   2.42132214,  16.86002753])
         y: array([  7.6608567 ,   6.09418351,   2.42132214,  16.86002751])',), <traceback object at 0x6af8638>)

    The only other failure is a NoOldAnswer failure for the new MHDCTOrszagTangAMR test.

    1. dcollins4096 author

      I tried to reproduce that error when Nathan ran into it, and wan't able to make it fail. Nathan concluded that it was an accumulation of roundoff error. Do you think that's the case for your run, or is this a real problem?

      1. Sam Skillman

        Honestly I probably wouldn't worry about it. It is a bit peculiar, but unless someone wants to really dive in on this, I'd say let's just keep it in mind and move on.

        1. dcollins4096 author

          I think we should revise that test. But that's for a different PR.

          Thanks for your help testing!

  6. Brian OShea

    OK folks, I think it's go time. To summarize:

    • the PR is now updated after merging dave's fork with the enzo-dev tip
    • enzo-dev with all of the MHD-CT changes passes the push suite, and produces bitwise identical results with the exception of Noh2DAMR, which is off in the ~8th decimal place for some people but not others. This is a bit mysterious, but has been deemed "not a show-stopper," and I don't see it on my own machine so I can't debug it.
    • We have a plan to move forward with the baryon field objects: the field objects are being implemented in enzo-3.0, and when the MHD is pulled from enzo-dev (really, the enzo-2.x development repository) into the enzo-3 development repository, it will be updated to include field objects.

    So, unless somebody has a substantial objection, I will accept this PR later this afternoon. Speak now or forever hold your peace! :-)


    1. MattT

      To be clear on point three, MHDCT will not be pulled into the named branch enzo-dev or the enzo-3.0 repository until it has been integrated with field objects.

  7. dcollins4096 author

    I just made one last-minute change to CommunicationCombineGrids that allocates ElectricFields if they're missing. This was causing runs without ParallelRootGridIO to segfault. Now I'll hold my peace.