Update moab frontend

[bam241]

PR Summary

PR Checklist

• pass black --check yt/
• pass isort . --check --diff
• pass flake8 yt/
• pass flynt yt/ --fail-on-change --dry-run -e yt/extern
• New features are documented, with docstrings and narrative docs
• Adds a test for any bugs fixed. Adds tests for new features.

This fixes the call to PyNE::mesh that now uses pymoab in place of pytaps

Thx to @kkiesling and @pshriwise for the help on this!

[welcome]

Hi! Welcome, and thanks for opening this pull request. We have some guidelines for new pull requests, and soon you'll hear back about the results of our tests and continuous integration checks. Thank you for your contribution!

[bam241]

@pshriwise @kkiesling do you know how to add labels ?

[neutrinoceros]

Hi @bam241 , thanks for this update !
FYI, only project members can manage labels, I'll do it for you :-)

[neutrinoceros]

I'm putting the "bug" label here but it'd be useful to have an example showing why the frontend is currently broken.
Also, I see the git history is a bit scrambled here, which makes reviewing harder than it should (there are a lot of files showing up in the diff that do not belong so it's easier to miss something you actually changed). Would you be able to rebase on the current tip of master ? If you're not confortable with the process I can do it for you, just let me know !

[kkiesling]

@neutrinoceros - the history should be fixed now.

[neutrinoceros]

Awesome ! Thanks for reacting so promptly !

[kkiesling]

Regarding how the frontend was broken. We updated PyNE back in January to use PyMOAB for meshes rather than PyTAPS. The methods called on the mesh in yt here were still the PyTAPS methods which are non-existent in PyNE now. We've updated them to the correct PyMOAB methods. (pyne mesh class for reference).

[matthewturk]

@kkiesling Thank you for doing this -- I (personally) had no idea that currently the Pyne frontend was broken, and we definitely want to rectify that!

[neutrinoceros]

Thanks for your reply ! It was brought to my attention that this is a known issue so an example isn't actually needed.
I've got a naive question before I start an actual review: this looks to me like it could maybe break some backward compatibility ? if it doesn't, great, otherwise we could have both versions of the affected code in and select which one to use with an if or a try statement.

[kkiesling]

By backwards compatibility, do you mean with older PyNE versions? It definitely does break compatibility with older release versions. We haven't had a PyNE release in a very long time so we are getting one ready this week. This works with the current develop branch of PyNE and will work with our in-progress PyNE release version. I would think it's okay to wait until that release is fully out to merge if you want it tied to a specific PyNE version (cc @gonuke if he wants to chime in on this). In my opinion, it is not worth supporting any older PyNE versions since those rely on PyTAPS which is not compatible with python 3. We don't support any PyTAPS anymore.

[matthewturk]

@kkiesling I think we would defer to your judgment on this topic, and we should not insist on backward compatibility if you believe the returns for the PyNE community are low.

[neutrinoceros]

we should not insist on backward compatibility if you believe the returns for the PyNE community are low.

I agree, was just curious how this would be received.

In my opinion, it is not worth supporting any older PyNE versions since those rely on PyTAPS which is not compatible with python 3

Well that settles it then, since yt isn't compatible with Python 2 anymore, there's is exactly 0 reasons to worry about backward compatibility indeed.

[kkiesling]

@matthewturk I don't think supporting backward compatibility here will be worthwhile.

[neutrinoceros]

I would think it's okay to wait until that release is fully out to merge if you want it tied to a specific PyNE version

Doesn't seems necessary since we're in between yt releases too, so that's on your end to judge whichever path you guys want to go.
I think we can always factor in more updates later if it turns out necessary.

[neutrinoceros]

As a note, I think it would be helpful to add a test of some sort here. It looks to me that the affected code isn't, and actually couldn't be tested as it is.

[neutrinoceros]

Left a couple of suggestions to improve code quality but overall this looks good to me !

[kkiesling]

As a note, I think it would be helpful to add a test of some sort here. It looks to me that the affected code isn't, and actually couldn't be tested as it is.

We may want a bigger discussion on this- adding tests would require building PyNE from source as a part of CI. Is this something the yt team would be okay with? If so, it would definitely make sense for this to wait until after we get our release out.

[neutrinoceros]

Yes, we discussed this with @munkm and indeed it's not as trivial as I naively assumed at first. I didn't realise how special this specific frontend is. Now I personally think it's probably not worth the maintenance cost for us to test this in here, unless it can be done through mock patches ?
Otherwise the testing part could probably be leveraged downstream in Pyne. Does this make any sense ? :)

[kkiesling]

We just discussed too and I think it makes sense for us to cover testing of the frontend in PyNE rather than here as it would not be trivial indeed. We don't rely on yt as a dependency in source except for visualizing data in tutorials (which is actually why this went unnoticed so long). But yes, we could come up with a better testing system on the PyNE side to catch these updates more frequently.

[neutrinoceros]

/black

[neutrinoceros]

I took the liberty of fixing a mistake introduced by my suggestion. Note that the black command affected a cookbook file as a side effect, which is okay. We don't regularly check format there but the bot doesn't know it.

[bam241]

running "/black" actually reformat the files ? that's cool !

[neutrinoceros]

@matthewturk just deployed this, we're still trying it out but seems to work !
you can use /format to apply black+isort+flynt too :-)

[munkm]

Thank you so much for this PR @bam241 @kkiesling and @pshriwise!! 🎉 ⚛️ 😎

[welcome]

Hooray! Congratulations on your first merged pull request! We hope we keep seeing you around! 🎆

[munkm]

Also, we're working towards a major release in the next few months. If you all notice any API discrepancies in yt infrastructure that you all are using please don't hesitate to let us know so we can try to address it. 🙂