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

Add failing test case for issue #549 (empty Importance object print) #681

Draft
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

kordusas
Copy link

@kordusas kordusas commented Mar 8, 2025

Pull Request Checklist for MontePy

Description

added pytest to teset the empty importance object printing

Fixes # (issue number)


General Checklist

  • I have performed a self-review of my own code.
  • The code follows the standards outlined in the development documentation.
  • I have formatted my code with black version 25.
  • 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.
  • For infrastructure updates, I have updated the developer's guide.
  • For significant new features, I have added a section to the getting started guide.

First-Time Contributor Checklist

  • If this is your first contribution, add yourself to pyproject.toml if you wish to do so.

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.

@MicahGale MicahGale self-requested a review March 8, 2025 19:11
@MicahGale MicahGale self-assigned this Mar 8, 2025
@MicahGale MicahGale added the bugs A deviation from expected behavior that does not reach the level of being reportable as an "Error". label Mar 8, 2025
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.

Thanks for the contribution @kordusas!

This is a good first pass but some changes are needed. I left comments on specific changes.

  1. Update the PR description to give a summary of what the problem that was solved is, and how it was solved. Also link to the specific issues. See Allow any Real type for floating point numbers and any Integral type for integer numbers during type enforcement #680 for an example.
  2. Do you want to add yourself as an author to AUTHORS and/or pyproject.toml?
  3. Make sure to update the changelog with this bug fix.
  4. Would you like to added as a contributor to this repo on GH?

Comment on lines +192 to +200
if self._problem:
# Use the full formatting (which relies on _problem.cells, etc.)
return "".join(self.format_for_mcnp_input(DEFAULT_VERSION))
else:
# Fall back to a default simple representation
default = self._format_default()
if default:
return " ".join(default)
return "Importance object is empty"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you should think beyond the previous implementation, and really think about what information is actually useful to the user here? Focus more on what information is important than reformatting the syntax tree.

Maybe something like

>>> str(imp)
Importance: {n: 1.0, p: 2.0...}

@tjlaboss has stronger opinions on these matters. Here's our current guidance: https://www.montepy.org/en/stable/developing.html#how-to-str-vs-repr

Comment on lines +35 to +38
def test_importance_init_empty(self):
importance = Importance()
assert str(importance) == "Importance object is empty"

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good start. I would like to see this test multiple scenarios though, and this test suite needs to be updated to be canonically pytest.

I'm thinking:

  1. Remove all use of TestCase, see this example: https://github.com/idaholab/MontePy/blob/alpha-test-dev/tests/test_material.py
  2. Make a pytest.fixture that sets up and returns the two cells from the previous test
  3. Test str and repr for:
    1. parsed cells from the fixture
    2. an empty importance,
    3. an importance/cell made from scratch

If this is too much infrastructure changes, let me know. I can do a PR onto your branch for some of it.

@MicahGale MicahGale marked this pull request as draft March 8, 2025 20:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugs A deviation from expected behavior that does not reach the level of being reportable as an "Error".
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants