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

Remove pymoab as a Dependency #24

Merged
merged 6 commits into from
May 30, 2024

Conversation

ahnaf-tahmid-chowdhury
Copy link
Member

Description

This PR removes pymoab from pyproject.toml as a dependency.

Changes

Here are the main changes made in this PR:

  1. pyproject.toml: Removed pymoab from the list of dependencies.

  2. dagnav.py: Added instructions to handle the case if pymoab is not found.

dagmc/dagnav.py Outdated Show resolved Hide resolved

try:
from pymoab import core, types, rng, tag
except ImportError as e:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know I suggested this in #1, but the message is a overly verbose for my taste. There are (somewhat dated) instructions on how to build MOAB with PyMOAB enabled here. I think we should point to them in this message and update the installation instructions there.

Also, while we may want to add a logging capability to this project in the future, I don't see the point of importing that module for this single message. We can raise our own message from a ModuleNotFoundError

try:
    from pymoab import core, types, rng, tag
except ImportError as e:
    msg = 'PyMOAB package was not found, please install this package using the instructions found here: https://ftp.mcs.anl.gov/pub/fathom/moab-docs/building.html'
    raise ModuleNotFoundError(msg) from e

Copy link
Member

@pshriwise pshriwise left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of comments form me, but I think this is close!

Copy link
Member

@gonuke gonuke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - I'll let @pshriwise decide if it's ready

@pshriwise pshriwise merged commit 112e4f4 into svalinn:main May 30, 2024
1 check 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