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

Material Specifications: cannot unpack non-iterable ValueNode object #365

Closed
SterlingButters opened this issue Feb 5, 2024 · 9 comments · Fixed by #368
Closed

Material Specifications: cannot unpack non-iterable ValueNode object #365

SterlingButters opened this issue Feb 5, 2024 · 9 comments · Fixed by #368
Assignees
Labels
bugs A deviation from expected behavior that does not reach the level of being reportable as an "Error". critical An issue that seriously limits user adoption or hampers current use. parsers are hard Examples of where MCNP syntax is complicated and should be simplified.

Comments

@SterlingButters
Copy link

SterlingButters commented Feb 5, 2024

@MicahGale How are material cards supposed to be formatted for MontePy to parse them? I keep getting the error in the subject line. When I inspect the isotope_fractions variable in \data_inputs\material.py, it is a ListNode object containing ValueNode objects. For any ValueNode object, I see no attribute that corresponds to the isotopic fraction (at least not one intuitively named). Additionally this object is non-iterable so the usage isn't correct either. Am I missing something?

@MicahGale MicahGale added the bugs A deviation from expected behavior that does not reach the level of being reportable as an "Error". label Feb 5, 2024
@MicahGale
Copy link
Collaborator

This seems like a bug most likely. MontePy should be able to handle a material definition as long as the parameters come at the end.

So this should work

m1 1001.80c 0.33
      8016.80c 0.666667
      plib=80p

This syntax breaks it:

m1 1001.80c 0.33
      plib=80p
      8016.80c 0.666667

These object types that you are seeing are data types from the syntax tree. A bad syntax should be handled with syntax errors. So this is definitely odd.

Could you provide an example made up material that causes this bug?

@MicahGale MicahGale added the question Further information is requested label Feb 5, 2024
@SterlingButters
Copy link
Author

Is the plib keyword required? I specify my materials like (without plib):

m1 1001.80c 0.33
      8016.80c 0.666667

@MicahGale
Copy link
Collaborator

No plib is not at all required. It was just an example of key-value parameter that you can use.

That looks like it should work. I'll have to look into this further.

@SterlingButters
Copy link
Author

SterlingButters commented Feb 6, 2024

I've never used SLY so I'm just throwing ideas at the wall but is the issue possibly linked to _flush_phrase never being called for the isotope_fractions (since _flush_phrase is what generates the ValueNode - although this doesn't make a ValueNode iterable but maybe you could zip the ZAID and fractions ValueNode's)?

@SterlingButters SterlingButters changed the title Material Specifications: cannot unpack non-iterable ValueNode object (Priority) Material Specifications: cannot unpack non-iterable ValueNode object Feb 6, 2024
@MicahGale
Copy link
Collaborator

MicahGale commented Feb 6, 2024

That example code I just dropped in MontePy and it didn't have any issues as I suspected. Could you get us a stack trace and MWE?

As for:

I've never used SLY so I'm just throwing ideas at the wall but is the issue possibly linked to _flush_phrase never being called for the isotope_fractions (since _flush_phrase is what generates the ValueNode - although this doesn't make a ValueNode iterable but maybe you could zip the ZAID and fractions ValueNode's)?

So _flush_phrase isn't a SLY thing, that's just a utility function I made. First this guide should give a quick intro to it. Basically _flush_phrase is just used for generating ValueNode as leaves of the tree quickly. isotopes_factions which is of type IsotopeNode is a list of pairs of ValueNodes that represent the ZAID and then the atom/weight fraction. So the issue is that somehow a ValueNode is getting put too high in the syntax tree that the Material object as it tries to walk the Syntax Tree is finding it instead of a IsotopesNode (which IIRC is a subclass of ListNode) for some reason.

Dynamic typing strikes again.

Test I ran:

In [1]: import montepy
 
In [2]: input = montepy.input_parser.mcnp_input.Input("""m1 1001.80c 0.33
   ...:       8016.80c 0.666667""".split("\n"), montepy.input_parser.block_type.BlockType.DATA)

In [3]: material = montepy.data_inputs.material.Material(input)

In [4]:

In [4]: material
Out[4]:
MATERIAL: 1 fractions: atom
 H-1   (80c) 0.33
 O-16  (80c) 0.666667

@MicahGale MicahGale changed the title (Priority) Material Specifications: cannot unpack non-iterable ValueNode object Material Specifications: cannot unpack non-iterable ValueNode object Feb 6, 2024
@MicahGale MicahGale added critical An issue that seriously limits user adoption or hampers current use. and removed critical An issue that seriously limits user adoption or hampers current use. labels Feb 6, 2024
@SterlingButters
Copy link
Author

SterlingButters commented Feb 6, 2024

@MicahGale Ahah! Found the issue - I lied to you. I did not, in fact, supply the .abx suffix - which, as you probably know:

If .abx is omitted the first entry from the xsdir file corresponding to the proper library class will
be used. This same approach will be used for all libraries not specified.

@SterlingButters
Copy link
Author

@MicahGale By the way, are we adding of these to your unit tests (or maybe these aren't worthy)?

@MicahGale
Copy link
Collaborator

So to confirm the problem is:

m1 1001 0.333
    8016 0.6667

Yes I know this absolutely valid and can't believe I missed this as a test.

So yes we will absolutely add this to our test suite. It is the expectation for any bug fixes. What I will usually do first is write/modify a test to replicate the bug. Ensure that it fails, then push that test to the PR so that way a record of the test failing is preserved. This just makes it nicer for the the reviewer ok really @tjlaboss to verify that the test is effective. I think 100% of the tests in tests/test_edge_cases.py comes from bug reports.

@MicahGale MicahGale added critical An issue that seriously limits user adoption or hampers current use. and removed question Further information is requested labels Feb 6, 2024
@MicahGale
Copy link
Collaborator

Error confirmed. Full stack trace:

---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
Cell In[3], line 1
----> 1 material = montepy.data_inputs.material.Material(input)

File ~/dev/montepy/montepy/data_inputs/material.py:40, in Material.__init__(self, input)
     38 set_atom_frac = False
     39 isotope_fractions = self._tree["data"]
---> 40 for isotope_node, fraction in isotope_fractions:
     41     isotope = Isotope(node=isotope_node)
     42     fraction.is_negatable_float = True

TypeError: cannot unpack non-iterable ValueNode object

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". critical An issue that seriously limits user adoption or hampers current use. parsers are hard Examples of where MCNP syntax is complicated and should be simplified.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants