ladisk / pyuff

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

Correct dataset 2412 #79

Closed nico-arnold closed 11 months ago

nico-arnold commented 1 year ago

Fix dataset 2412 naming as discussed

nico-arnold commented 1 year ago

Hey @jankoslavic , I just noticed the pytest is inconsistent now here. Will correct and update.

nico-arnold commented 1 year ago

Testing fixed. I can also correct the test to convert dataset 2412 to 82 - better in a new PR?

jankoslavic commented 1 year ago

@nico-arnold thank you for your effort. Few comments:

nico-arnold commented 1 year ago

Hey @jankoslavic ,

1.) Ok, no problem. But please keep in mind that there are a looot of more elements that just triangles and quads. What would be the best way to name those? In fact there are also different types of every kind of element because additional to linear ones most also have a quaratic an cubic implementation: https://victorsndvg.github.io/FEconv/formats/unv.xhtml

That's why I assumed it would be the best to handle them as the IDs they represent and also changed the tests because the indices "quads" and "triangles" don't exist here, instead they are called "33" and "34". Also, there is a list "all" to adress all elements. Do you have a good idea how to keep this one in sync when somebody changes an element, for example in dset["quads"]? Then the "all" group also has to be changed and vice versa. Maybe it's the best do just keep them as an array of elements instead of groups, and the users have to decide by themselves how they sort them? Or maybe methods like get_quads(), write_quads() etc.? And if yes, do linear and quadratic quads for example go into the same group here?

2.) Yes, that makes sense. But I didn't have an idea for a good way to implement that in the way you did. Rods need 3 lines of code, and all other elements need 2 lines. So I thought it might be the best way to solve it this way because I can assure every element gets handled in the way it should be. If all elements were represented by the same umber of lines I would definitely have kept your implementation (that is a great way to do that BTW)

3.) Okay, no problem, good point. I just thought it might be better to have a test file that tests more element types instead of just quads. Will correct that (:

jankoslavic commented 1 year ago

@nico-arnold Thank you! for 2): We have first to take care of the accuracy and the speed. I understand your implementation is much more general and it ok to continue with it. it is ok with me to continue with your implementation! Will wait for your commits and then continue!

nico-arnold commented 1 year ago

Okay, great! How about point 1? Same for that or d you have a different idea how we could label the single element groups?

jankoslavic commented 1 year ago

For point 1: I would follow your suggestion; if possible, we should not break backward compatibility. Is there a reference for the UNV descriptor/name and FE type at this link: https://victorsndvg.github.io/FEconv/formats/unv.xhtml?

nico-arnold commented 1 year ago

@jankoslavic Great! Sadly I don't have further reference for this one except this post here: https://www.eng-tips.com/viewthread.cfm?qid=30398&msclkid=f1d23c5dcde911ecb0c1233ee226a8dc Otherwise, no official documentation from SDRL or anything. Or, more precisely, not anymore I guess.

jankoslavic commented 1 year ago

@nico-arnold thank you. I would suggest trying to built on the data available, also the last link. Are you able to update the PR?

nico-arnold commented 1 year ago

Noted, will do. So I will keep 'quads' and 'triangles' for backward compatibility; only the linear ones? Or should I summarize all quads (also quadratic and cubic ones) and also triangles in those arrays? I have much going on the next days, so I can't spend too much time here, but will try to get everything updated this week.

jankoslavic commented 1 year ago

Dear @nico-arnold thank you! I would keep the backward compatibility for linear, only (and can be depreciated in a few years). The implementation you are proposing is more general and should be the proper way of usage in the future. How does this sound to you?

nico-arnold commented 1 year ago

Hey @jankoslavic - sounds great to me (:

nico-arnold commented 12 months ago

Ready to merge

jankoslavic commented 12 months ago

For me, this is ready to be merged. I would just wait for a while with regard to this related PR: https://github.com/ladisk/pyuff/pull/82

jankoslavic commented 11 months ago

This can now be merged. @nico-arnold thank you!