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.

Sign up for GitHub

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

Jump to bottom

Make Particle Type errors into a warning and a NotImplementedError #656

Merged
merged 10 commits into from
Feb 8, 2025
Merged

Make Particle Type errors into a warning and a NotImplementedError #656

merged 10 commits into from
Feb 8, 2025

Conversation

tjlaboss
Copy link
Collaborator

@tjlaboss tjlaboss commented Feb 7, 2025 •
edited by MicahGale
Loading

Pull Request Checklist for MontePy

Description

ParticleTypeNotInProblem and ParticleTypeNotInCell are fatal errors. These exceptions arise when the importances of the cells do not agree with those of the run mode. The MCNP6.3 manual does not indicate that these mismatches necessarily result in an unrunnable MCNP model. Therefore, these exceptions have been changed from errors to warnings.

This PR also converts the entirety of test_importance.py from unittest to pytest.

Fixes #381

General Checklist

  • I have performed a self-review of my own code.
  • The code follows the standards outlined in the development documentation.
  • I have added tests that prove my fix is effective or that my feature works (if applicable).

Documentation Checklist
  • I have documented all added classes and methods.

Additional Notes for Reviewers

Ensure that:

  • The submitted code is consistent with the merge checklist outlined here.
  • The PR covers all relevant aspects according to the development guidelines.
  • 100% coverage of the patch is achieved, or justification for a variance is given.

📚 Documentation preview 📚: https://montepy--656.org.readthedocs.build/en/656/

@tjlaboss
New ParticleTypeWarning as parent class
72958e5
@tjlaboss
Treat ParticleType exceptions as warnings
228272f
@tjlaboss
Convert test_importance.py from unittest to pytest
a159e8c 
Also, throughout, 'assertEqual' and 'assertAlmostEqual' were used
interchangeably to test floating-point importances.
I have changed them all to be 'assert x == y'.
Alternatively, one could compare values with 'pytest.approx'
@tjlaboss
Unit test for warning, not error
63ab5ee
@tjlaboss
Copy link
Collaborator Author

tjlaboss commented Feb 7, 2025

@MicahGale what version of black does the MontePy CI use now?

@tjlaboss tjlaboss requested a review from MicahGale February 7, 2025 18:50
@tjlaboss
Copy link
Collaborator Author

tjlaboss commented Feb 7, 2025

Regarding the #TODO, see MCNP 6.3 Manual § 5.12.1:

image

@MicahGale MicahGale added the feature request An issue that improves the user interface. label Feb 7, 2025
@MicahGale
Copy link
Collaborator

@MicahGale what version of black does the MontePy CI use now?

It always installs the latest stable from pypi, which is black 25.1.*. I'd say just do pip install -U black

@tjlaboss
Copy link
Collaborator Author

tjlaboss commented Feb 7, 2025

Going to leave this as a draft while we think about the way to implement this logic. Raising a warning for every cell without an importance set is annoying (a single warning is still appropriate).

Something like the following seems to make sense:

  • If the importance for a cell is not set, set to None, NaN, or a sentinel value. I suggest None.
  • In the global scope, check if all cells have at least one importance. If not, raise an error.
  • Check for the presence of a WWN card. If so, assign missing importances to 0 where all given importances are 0. Then, assign all other missing importances to 1.

There is some additional complicated logic that worthy of at least one independent issue. Perhaps, for the scope of this PR, the unspecified importance should just be set to None or Jump, or be returned to raising a fatal error.

@MicahGale
Copy link
Collaborator

There is some additional complicated logic that worthy of at least one independent issue. Perhaps, for the scope of this PR, the unspecified importance should just be set to None or Jump, or be returned to raising a fatal error.

So in MontePy None == J for a ValueNode.

However, this will have different consequences:

From section 5.12.1 further on:

If a cell importance is set to 0 for any particle, all particle importance for that cell will be set to 0 (default implicit value) unless specified otherwise. However, if the nJ feature is used to specify importance, the values jumped over are given a default importance value of 0.

I think for now a fatal Error will have the lowest unintended consequences to users.

@tjlaboss tjlaboss mentioned this pull request Feb 7, 2025
Open
@tjlaboss
Copy link
Collaborator Author

tjlaboss commented Feb 7, 2025

I agree. Related issue #657 has been opened for full default importance logic implementation.

@MicahGale
Copy link
Collaborator

I agree: split #657 as separate scope. Constrict this scope to just parsing-time warnings.

@tjlaboss tjlaboss marked this pull request as ready for review February 7, 2025 21:07
@tjlaboss tjlaboss changed the title Draft: Make Particle Type errors into warnings Make Particle Type errors into a warning and a NotImplementedError Feb 7, 2025
@tjlaboss
Copy link
Collaborator Author

tjlaboss commented Feb 7, 2025

Went for a NotImplementedError until #657 is implemented.

MicahGale
MicahGale requested changes Feb 7, 2025
View reviewed changes
Copy link
Collaborator

@MicahGale MicahGale left a comment

Choose a reason for hiding this comment

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

The patch coverage is 85%. I need to investigate further what is missed in coverage.

@tjlaboss
Copy link
Collaborator Author

tjlaboss commented Feb 7, 2025

ParticleTypeNotInCell is never raised and therefore never tested. The new NotImplementedError also gets another line of text that isn't run. We could add a test for it, and then later replace that test once it is implemented.

@MicahGale
Copy link
Collaborator

ParticleTypeNotInCell is never raised and therefore never tested. The new NotImplementedError also gets another line of text that isn't run. We could add a test for it, and then later replace that test once it is implemented.

Make it so.

@tjlaboss tjlaboss requested a review from MicahGale February 7, 2025 23:12
MicahGale
MicahGale reviewed Feb 7, 2025
View reviewed changes
tests/test_importance.py Outdated
with pytest.raises(MalformedInputError):
montepy.read_input(os.path.join("tests", "inputs", "test_imp_redundant.imcnp"))


def test_not_imp():
Copy link
Collaborator

Choose a reason for hiding this comment

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

Switch to a more meaningful test name. I am going to be so confused by this in the future. Maybe: test_not_implement_on_export

MicahGale
MicahGale requested changes Feb 7, 2025
View reviewed changes
Copy link
Collaborator

@MicahGale MicahGale left a comment

Choose a reason for hiding this comment

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

Just need a clearer test name.

@tjlaboss tjlaboss requested a review from MicahGale February 7, 2025 23:42
@tjlaboss tjlaboss merged commit 8072131 into alpha-test-dev Feb 8, 2025
24 checks passed
@tjlaboss tjlaboss deleted the parser_type_warn branch February 8, 2025 17:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MicahGale MicahGale MicahGale approved these changes

Assignees
No one assigned
Labels
feature request An issue that improves the user interface.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tjlaboss @MicahGale