ladisk / pyuff

This module defines an UFF class to manipulate with the UFF (Universal File Format) files.
Other
58 stars 40 forks source link

FEA support #65

Closed theScriptingEngineer closed 1 year ago

theScriptingEngineer commented 1 year ago

Hello @jankoslavic ,

I have added support for:

jankoslavic commented 1 year ago

@theScriptingEngineer great PR; thank you!

Can you push it slightly further?

  1. With regard to 2414, record 3:

    Record 3:        FORMAT (1I10)
                 Field 1:      -- Dataset location
                                   1:    Data at nodes
                                   2:    Data on elements
                                   3:    Data at nodes on elements
                                   5:    Data at points

    I would suggest the if sentence to start with 1: if dset['analysis_type'] == 1 and then go to 5. If a particular option is not yet developed we can "pass" it. Now we have 5 and else, the else part is hard to understand for humans, e.g. record10_field1, record10_field2,... Would you be able to simplify this with regard to the documentation at https://www.ceas3.uc.edu/sdrluff/?

  2. please add testing for the new functionality of 2414.

  3. please add testing for the 2429.

How does this sound?

jankoslavic commented 1 year ago

Hi @theScriptingEngineer so you need help?

theScriptingEngineer commented 1 year ago

Hi @jankoslavic,

Regarding the if dset['analysis_type'] == 1 (record 9, field 2 ranging from 0 to 9): I think you are mixing with dset['dataset_location'] (record 3, ranging from 1 to 5, excluding 4). I agree that I should add 5 in the if sequence and pass it for record 3. But the parsing of 2414 should only depend on record3, not on record 9, field 2. I kept this dependency in order not to break your code. Records 10-13 eg. record10_field1 cannot be assigned a name, since this will depend on the application reading the file. I my case (where Siemens NX is the application) I would have totally different dictionary keys.

I am ok to implement. I do not have the time to do so at this moment, but will try to implement by the end of the year.

jankoslavic commented 1 year ago

Dear @theScriptingEngineer thank you for the comments. Yes, it should be dset['dataset_location']. It is a very good PR. No problem waiting a few weeks more. Thanks

theScriptingEngineer commented 1 year ago

Hi @jankoslavic,

I have added the tests for dataset 2429, which I had added earlier.

For dataset 2414 I also added additional test for the "general" treatment of the universal file. I have added 2 universal files in the 'data' folder, which I used in testing. One of these files also contains dataset 2411 and 2412. Feel free to use these files for other development.

I have tested all 'dataset_location' options:

I did this by adding 2 additional defs in test_2414.py and adding them to the main.

I pushed everything to the FEASupport branch, so they are included in this pull request.

Regards, Frederik

PS: good thing you made me test, still found a bug ;-)