Implementing Read Cards - [merged]

[MicahGale]

Merges issue5ReadCard -> develop

[MicahGale]

changed target branch from main to develop

[MicahGale]

added 2 commits

• 9d7100ad - Removed diagnostic print statements
• cbd8edb1 - First pass at implementing read card

Compare with previous version

[MicahGale]

added 1 commit

• 1d820d61 - Fixed weird generator bugs by making to_read list

Compare with previous version

[MicahGale]

added 2 commits

• 5bd63565 - Fixed negative material fraction bug
• 0559f6a0 - Merge branch 'materialIssue' into issue5ReadCard

Compare with previous version

[MicahGale]

added 3 commits

• aaf03840 - Made cells surfaces and materials sortable
• 7ec3ed0e - Made title easily settable
• 5f212ad0 - Made way to add children objects of cells to problem

Compare with previous version

[MicahGale]

In GitLab by @tjlaboss on Jan 24, 2022, 17:51

Commented on mcnpy/input_parser/input_syntax_reader.py line 65

def read_data(fh, block_type: int=None, recursion=False):

Docstring needs to include block_type and recursion.

[MicahGale]

In GitLab by @tjlaboss on Jan 24, 2022, 17:51

Commented on mcnpy/input_parser/input_syntax_reader.py line 143

I think this will work as written. Let's verify that with a test case at least 3 files deep.

[MicahGale]

In GitLab by @tjlaboss on Jan 24, 2022, 17:51

Commented on mcnpy/input_parser/mcnp_input.py line 39

        self._words = words
        self._block_type = block_type

Misuse of private variables. Only use double underscore if children should not have access to these attributes. Otherwise, use single-underscore for private variables.

[MicahGale]

In GitLab by @tjlaboss on Jan 24, 2022, 17:51

Commented on mcnpy/input_parser/mcnp_input.py line 74

        self._words = card.words
        self._block_type = card.block_type

Don't use name mangling except for debugging.

[MicahGale]

In GitLab by @tjlaboss on Jan 24, 2022, 17:51

Commented on mcnpy/input_parser/mcnp_input.py line 70

I don't understand the inheritance here: if ReadCard is a Card, why not keep its parent's __init__?

[MicahGale]

In GitLab by @tjlaboss on Jan 24, 2022, 17:51

Commented on mcnpy/mcnp_problem.py line 250

        self.__surfaces += surfaces
        self.__materials += materials
        self.__data_cards += materials

Do these need to be double-underscore privates?

[MicahGale]

In GitLab by @tjlaboss on Jan 24, 2022, 17:53

The recursive read function looks good on inspection. I'll have to play around with a test case and see if it behaves as expected.

The suggested changes are primarily on the use of private variables.

[MicahGale]

You mean having three levels of files having read cards?

I really need to start a testing plan. Is integration testing the main way to go?

[MicahGale]

Yes I learned this the hard way with subclassing. At this point it's just Protected right?

[MicahGale]

The goal with the init is to extract the filename. I'm not tackling this the best way I agree. Are you suggesting the same function signature and then using super?

[MicahGale]

You saw right through my first attempt at private variables ever.

[MicahGale]

In GitLab by @tjlaboss on Jan 25, 2022, 11:57

Commented on mcnpy/input_parser/mcnp_input.py line 70

Yes.

[MicahGale]

In GitLab by @tjlaboss on Jan 25, 2022, 12:01

Commented on mcnpy/input_parser/mcnp_input.py line 39

Essentially yes. Python doesn't officially use Private/Protected terminology.

[MicahGale]

In GitLab by @tjlaboss on Jan 25, 2022, 12:03

Generally speaking, I always use single-underscores for _private variables. It's pretty rare when something needs to be name-mangled __private.

[MicahGale]

In GitLab by @tjlaboss on Jan 25, 2022, 12:07

Commented on mcnpy/input_parser/input_syntax_reader.py line 143

Right. File 1 reads File 2, which reads File 3.

Good practice is to start with unit tests. I am ~sometimes~ usually lazy and start with just one big integration test, filling in the unit tests once it starts working.

[MicahGale]

added 1 commit

• 8e6545b2 - Fixed bug with updating surfaces

Compare with previous version

[MicahGale]

marked this merge request as ready

[MicahGale]

block type is actually an enum, noted.

[MicahGale]

In GitLab by @tjlaboss on Feb 19, 2022, 11:20

This needs !6. Let's apply that and then merge.

[MicahGale]

This is covered by tests.test_integration.test_read_card_recursion in !11.

[MicahGale]

And I think !6 needs !8 and !10 to merge safely. So I'm thinking: !10 + !8 -> !6 -> !4.

[MicahGale]

I think this should be covered now.

[MicahGale]

Covered by !6.

[MicahGale]

Should be included in !6

[MicahGale]

changed this line in version 7 of the diff

[MicahGale]

changed this line in version 7 of the diff

[MicahGale]

changed this line in version 7 of the diff

[MicahGale]

added 33 commits

• f300e99a - Started writing unit tests
• 4f2c3271 - Added test cases for finding all types of mcnp input syntaxes
• 5a813cc7 - Wrote most of the syntax parsing test cases
• c53dd9d5 - Reworked the ReadCard init function to be consistent
• 110d6d46 - Tested if vertical mode throws an issue
• a925e312 - Moved error to be under notImplementedError
• 4b72af35 - Added terminus comment for a block to test
• 834510ae - Fixed typo
• 24cf0838 - Fixed vertical mode test case to actually use the syntax
• a88cb1a5 - Added CARD str testing
• 407104f2 - Fixed blockID testing to get final card
• 98d70dff - Fixed failing string repr tests
• 5249dfaa - Fixed message block finder to be case insensitive
• 879834bd - Fixed card sequence to match actual test case
• 517e1d05 - Fixed ampersand continue finder
• 778d0561 - Fixed bug with 0 length Cards
• 5ea3b34b - Fixed issues with read card.
• a0ab837a - Added tests for the fortran float parser
• 2be5dad6 - Wrote most of the cell test cases
• c5f517d4 - Fixed typo
• 11ce367a - Added sorting test for cells
• 7a7e1f13 - Wrote test cases for the data cards
• 1c106d2e - Fixed words typo
• d359d2da - Wrote data parser test case
• 0117f07d - Wrote test cases for most of the material affiliated classes
• 0cc63452 - Added transform matrix
• 033ff4c3 - Fixed transform comparison tests
• 3121d8b8 - Made tests for surface
• ad5e99a2 - Added init test for wrong surface type
• 049f4204 - Added tests for other surface objects
• 78f1b922 - Started writing integration and problem tests
• 3a54cdb0 - Fixed small typo
• 3639e523 - Merge branch 'testing' into 'issue5ReadCard'

Compare with previous version

[MicahGale]

added 29 commits

• d7e3a612 - Fixed parameters to be all upper case
• 12a11e99 - Removed useless method
• 2a8becc9 - Fixed thermal scattering init bug
• f1c3132d - Added transform matrix
• 802331be - Fixed transform comparison tests
• 4c57b2d7 - Made tests for surface
• 11799f74 - Added init test for wrong surface type
• 52a317eb - Added tests for other surface objects
• 4977e442 - Started writing integration and problem tests
• 32fb65d5 - Fixed small typo
• 72997349 - Corrected object instance for numpy array
• ef0446a1 - Changed how comparison function checks if a rotatio matrix is empty
• 6df875bc - Ensured that surface numbers are always positive
• 6911223c - Caught bad number parsing for the periodic/transform numbers
• 84f7a3b4 - Moved __ to _ as private for surface constants
• 64768804 - Fixed nonexistant title state
• 0ef01f86 - Test words in uppercase.
• 62483336 - Test more implemented surfaces
• a72b78fe - This was bothering me.
• 884f8315 - 'i' is in use. Avoid confusion.
• 8a979278 - Test nested read cards
• 4675c658 - Test an even stupider float parse.
• 4b00e875 - The ident case swapping caused a keyerror
• d36f4505 - Made everything pretty with Black
• 5ba48c0a - Revert "Test nested read cards"
• f98efec0 - Merge branch 'tjlaboss-fixUnit'
• fd739698 - Merge branch 'tjlaboss-fixUnit' into 'fixUnit'
• b28fe4ef - Merge remote-tracking branch 'origin/issue5ReadCard' into fixUnit
• 82aa26bd - Merge branch 'fixUnit' into 'issue5ReadCard'

Compare with previous version

[MicahGale]

changed this line in version 9 of the diff

[MicahGale]

changed this line in version 9 of the diff

[MicahGale]

added 13 commits

• 1ac46479 - Change name-mangled __private to _private attributes.
• da609510 - Merge branch 'issue5ReadCard' into __issue5ReadCard
• 72a4b284 - Removed old merge conflict
• f05828af - Fixed cell Paremeters property function for __names
• 92d3a5d8 - Fixed issue with updating link to periodic surface
• b53ccd0e - Reformatted with black
• 5f2bd26c - Fixed thermal scattering parent updating call
• 38c34c6b - Fixed MCNP problem message testing
• d4afb8bc - Fixed comment for mcnp_card
• b75ca18a - Fixed thermal scattering init bug
• a6c53f29 - Fixed periodic surfuce init function
• bb11aa18 - Removed last hasattr call
• 0b2a050c - Merge branch '__issue5ReadCard' into 'issue5ReadCard'

Compare with previous version

[MicahGale]

added 1 commit

• 6bda4b48 - Added docstrings for the read_data function changes

Compare with previous version

[MicahGale]

added 1 commit

• 3fb99a04 - Switched to add assign notation

Compare with previous version

[MicahGale]

Is this resolved by: 6bda4b48?

[MicahGale]

Resolved by !6 and 3fb99a0.

[MicahGale]

mentioned in commit f5ba9b44bba752d4da5230e06f09107972ac7632