-
Notifications
You must be signed in to change notification settings - Fork 129
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
FEAT: DXF import updates #5523
FEAT: DXF import updates #5523
Conversation
Thanks for opening a Pull Request. If you want to perform a review write a comment saying: @ansys-reviewer-bot review |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5523 +/- ##
==========================================
+ Coverage 84.90% 84.92% +0.02%
==========================================
Files 144 144
Lines 60628 60628
==========================================
+ Hits 51476 51490 +14
+ Misses 9152 9138 -14 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SMoraisAnsys Please could you check if the added dependency adds any vulnerabilities?
I think we try to minimize adding new dependencies, mainly for the main ones
@isaacwaldron I can't have a look right now but could you let us know if there are other value to add this dependency except for the read method ? As you know, adding a dependency can add a lot of issues regarding dependencies, maintenance, security, ... So I would rather not add a new dependency if we can avoid it. For example, we recently found that had a problem to install |
@SMoraisAnsys @Samuelopez-ansys I don't see any immediate additional benefit unless in the future we decide to add more DXF-focused features that require manipulation of these files. I decided to go ahead with I can relatively easily revert the addition if we prefer not to add the dependency. |
@isaacwaldron Thanks for the feedback, I'll have a better look on this potential dependency. @Samuelopez-ansys Let us discuss next week on that subject as I don't know much about how deeply we leverage DXF in pyaedt. |
@isaacwaldron I have pushed a few modifications, please take a look and let us know if you think this could impact your workflow |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreeing with Samuel's changes. We don't expect many people needing this dependency so I would prefer not having it as a default dependency of PyAEDT.
We'll discuss on creating groups of dependency that make more sense in a near future.
Could you see the changes and let us know ?
@Samuelopez-ansys @SMoraisAnsys the customer reporting this has previously complained about the install weight of PyAEDT, which resulted in some of the current libraries in all being removed from the base set back when this originally happened. I welcome additional discussion on dependency groups but for now I think restoring the original custom approach with a modification to support the DXF R12 layer table is best since we do not want this in the base set. |
Is the problem of installation associated to dependencies or to the PyAEDT repo itself ? I don't think we have that many dependencies in |
60251a8
60251a8
to
de4f973
Compare
@SMoraisAnsys the customer's concern originally was that libraries like matplotlib, pyvista, etc. were in the base set but if you are not planning to do any plotting this significantly increases the download and install weight. |
The user can still do: pip install pyaedt And it will work with a minimum size of venv. Right now, we can not add ezdxf to the basic dependencies of PyAEDT, because we are trying to minimize it (if possible). |
Makes sense. We'll probably move to having a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't check but I assume that old_dxf.dxf is R12 format. Can you rename the file so that we can know that from the file name ?
1. Use ezdxf instead of a homebrewed solution to detect the plot layer names from DXFs for import. 2. Relax restriction of DXF import in non-graphical mode as it seems to work in 2024 R2 at least. 3. Enable using the modeler's default tolerance for DXF self-stitching by supplying a negative value for self_stitch_tolerance.
Restore the original custom implmentation for reading DXF layer names with an extension to support DXF R12 format files.
Incorporate feedback about new test file name and adjust the import DXF test to check both the modeler-default and specified-tolerance stitching behavior.
a58ec32
to
8e8f790
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Description
Issue linked
Fixes #5520
Closes #5521
Closes #5522
Checklist