-
Notifications
You must be signed in to change notification settings - Fork 50
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
Minimum install for tidy3d #2087
Conversation
This version works with the following requirements.txt. In an empty python environment, I can succesfully do on the import tidy3d as td
sim = td.Simulation.from_file("sim_to_validate.hdf5") |
45a7912
to
d8c8ab2
Compare
@majinge7846 can you check if this works for you? scipy is not necessary anymore for the interp validation. So installing and running according to my above comment should work. |
5d341dc
to
403d211
Compare
from matplotlib import pyplot as plt | ||
from matplotlib.tri import Triangulation | ||
except ImportError: | ||
pass |
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.
Should we print a warning for users?
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.
question goes out to @momchil-flex 😄
I think we discussed this previously and decided not to?
# default arrow style | ||
arrow_style = ArrowStyle.Simple(head_length=12, head_width=9, tail_width=4) | ||
except ImportError: | ||
arrow_style = None |
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.
This won't break anything elsewhere?
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.
Yeah good point. Changed to a default that won't error in here (which is the only place where that default style is used):
tidy3d/tidy3d/components/geometry/base.py
Lines 2232 to 2239 in 78dfeff
arrow = patches.FancyArrowPatch( | |
(x0, y0), | |
(x0 + v_x, y0 + v_y), | |
arrowstyle=arrow_style, | |
color=color, | |
alpha=alpha, | |
zorder=np.inf, | |
) |
In general the purpose of this change was to make this file not error on import if matplotlib isn't available, in which case the script otherwise would have failed when calling (an undefined) ArrowStyle
. It's kind of expected that the code would still fail if the function that uses this arrow_style
is called at any point. However, this should not happen because the only purpose of this minimal install PR is to give MC a way to import a version of tidy3d with minimal requirements and run validation.
Just curious how you tested this fully worked? Trust that it does it's mainly a validation question. |
Like this: #2087 (comment) Note that none of this is supposed to be seen or used by users, and we definitely still need a better core packaging / refactor solution. |
I see, so this just tests that a full simulation can be reloaded in terms of the new package imports. However, I guess I was curious if we should test for the plotting functionality at some point. But really this is just whether the environment properly works or not, and for a user without the packages installed it'd error whenever one of the plotting methods is called. So this satisfies the packaging import test, (and in principle the new import structure shouldn't affect the internal plotting methods if matplotlib is installed), and if matplotlib is not installed the plotting method will simply not work but we are not warning users explicitly until they call a method. I wonder if the error in this case when a user doesn't have matplotlib installed will be clear enough, or whether we just let them sort it out on their own. However, this PR is already good in terms of getting us closer to the core refactor plan! If we think we don't have to worry about these user plotting methods errors (it kind of depends how unclear and frustrating these can be?) then it's probably good to go! And if any of these plotting methods affect any other commonly used methods. This said, think it's probably fine as is imo. |
Yannick and I spoke quickly and yeah all good on this! Now see how it fits in, just still catching up with things. |
Feel free to merge @yaugenst-flex whenever you want |
Shifting some things around to identify minimal set of packages needed to be able to validate a sample simulation file.
Currently requires: