search

ladisk / pyuff

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

Support for dataset 2467; Fix datasets 2412 and 2420 #74

Closed nico-arnold closed 12 months ago

nico-arnold commented 1 year ago

Hey, I needed a library to read UNV files including groups and modified your project a bit:

Maybe you find those useful. Thanks a lot for your great work!

jankoslavic commented 1 year ago

Thank you @nico-arnold ! This is a big PR; would it be better to separate it into smaller PRs. Would it be possible to add testing for dataset 2467? Why is it required to change the beam.uff test data? I would suggest leaving the current data "as is" as the current tests should pass without problem also after PR. Is this not the case?

nico-arnold commented 1 year ago

Hey @jankoslavic ! Oh, indeed, sorry for that; I'm new to working with git and will tidy that up/split into multiple PRs this week. Test implementation should also be no problem, I can do that. About the beam.uff: I corrected the file as it is - at least in my understanding - not complying with the UFF specification. Line 81 "11 0 8" implies there is a new record for a CS starting (R3 F1-3) with the ID 11, but there is no further data given. The matrix just before that defines the parameters of the CS before with the ID 11, so line 81 is redundand and shouldbe removed. See here for the UFF specification: https://www.ceas3.uc.edu/sdrluff/ (direct links are sadly not supported; just type "2420" in the search)

jankoslavic commented 1 year ago

Dear @nico-arnold! Thank you. If you could make smaller PRs this would be great. If the test file is not ok, a separate PR would be great to have!

However, for 2420, the specification is given below. To me everything look ok. Or? Thank you for your effort! Janko

Name: Coordinate Systems

Record 1: FORMAT (2I10) Field 1 -- Part UID

Record 2: FORMAT (40A2) Field 1 -- Part Name

Record 3: FORMAT (4I10) Field 1 -- Coordinate System Label Field 2 -- Coordinate System Type = 0, Cartesian = 1, Cylindrical = 2, Spherical Field 3 -- Coordinate System Color

Record 4: FORMAT (40A2) Field 1 -- Coordinate System Name

Record 5: FORMAT (1P3D25.16) Field 1-3 -- Transformation Matrix Row 1

Record 6: FORMAT (1P3D25.16) Field 1-3 -- Transformation Matrix Row 2

Record 7: FORMAT (1P3D25.16) Field 1-3 -- Transformation Matrix Row 3

Record 8: FORMAT (1P3D25.16) Field 1-3 -- Transformation Matrix Row 4

Records 3 thru 8 are repeated for each Coordinate System in the Part.

nico-arnold commented 1 year ago

Hello Janko,

sorry for my delay. Yes, I'm pretty sure it should be in the way that I corrected it. Every single CS record starts with a R3 (consisting of 3 int fields) and ends with R8 (consisting of 3 float fields for the matrix). The records for the coordinate system "CS10" en with line 80 (the last 3 values for its matrix), and the line consisting of 3 ints would be the start of a new CS (following the order, that would be "CS11") with the id "11" already. So to stay within the format, it would be necessary to add the other records 4-8 that define the name and matrix for CS11.

I'm cherry-picking commits right now to split everything up in 3 pull requests: 1) correct dataset 2412 2) add support for dataset 2467 3) correct the line inside the "beam.uff" file

Would that be okay? I will do the testing tomorrow when I'm done picking, I'm new to git so that's all a learning process for me. Do you already want a PR for the dataset 2467 or better a later one when I implemented the testing?

best wishes an have a great week, Nico

jankoslavic commented 1 year ago

Thank you @nico-arnold !

Please continue your effort as you plan! This would be great!

Regarding the beam.uff: Records 3 thru 8 are repeated for each Coordinate System in the Part. The record3 is before record4, not after record8. This is the last coordinate system:

        10         0         8
CS10
  -4.4807361612917013e-01   0.0000000000000000e+00   8.9399666360055785e-01
  -0.0000000000000000e+00   1.0000000000000000e+00   0.0000000000000000e+00
  -8.9399666360055785e-01  -0.0000000000000000e+00  -4.4807361612917013e-01
   0.0000000000000000e+00   0.0000000000000000e+00   0.0000000000000000e+00

YES, you are right, the line:

        11         0         8

has to be removed.

Thank you for the explanation and additional effort!

nico-arnold commented 1 year ago

Hello @jankoslavic ,

sure, you're welcome, I'm happy I can help!

Pull requests are up now. I merge all 3 branches into a branch "test" and then did "git dig master test" an got the following:

`diff --git a/README.rst b/README.rst index 7685940..626308e 100644 --- a/README.rst +++ b/README.rst @@ -1,6 +1,3 @@ -!!This is a fork from pyuff with support for dataset 2467. See link for the original library; note that this version can't be installed via pip.

- pyuff

@@ -8,7 +5,7 @@ Universal File Format read and write

This module defines an UFF class to manipulate with the UFF (Universal File Format) files.

-Read from and write of data-set types 15, 55, 58, 58b, 82, 151, 164, 2411, 2412, 2414, 2420, 2429, 2467 are supported. +Read from and write of data-set types 15, 55, 58, 58b, 82, 151, 164, 2411, 2412, 2414, 2420, 2429 are supported. ` According to my understanding that should mean that the merged sum of all 3 branches should now be up-to-date with my previous commit from my "master" branch, correct? I will implement testing for dataset 2467 and then send the final PR for that one.

jankoslavic commented 12 months ago

@nico-arnold I will close this PR as we have handled it in split PR. If I have missed anything, please re-open. Thanks! Great job.