Skip to content
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

Changes to _eval_step_func #2895

Merged
merged 11 commits into from
Sep 5, 2024
Merged

Changes to _eval_step_func #2895

merged 11 commits into from
Sep 5, 2024

Conversation

kombatEldridge
Copy link
Contributor

@kombatEldridge kombatEldridge commented Aug 27, 2024

In my project, I am wanting to use some step functions while Meep runs. Currently, my entire simulation of Meep runs in a Simulation class and I would like to keep my step functions in this class as well. That being said, they must have self as an arg. With the current _eval_step_func, the arg handler is a bit too strict. There is probably a reason for this, and please disregard this PR if so.

The changes I am requesting would include the possibility that the step functions contain an arg named self but should still keep the intended limitations of the previous version.

@codecov-commenter
Copy link

codecov-commenter commented Aug 27, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 83.33333% with 1 line in your changes missing coverage. Please review.

Project coverage is 73.71%. Comparing base (f29a8c7) to head (566c47d).
Report is 33 commits behind head on master.

Files with missing lines Patch % Lines
python/simulation.py 83.33% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2895      +/-   ##
==========================================
- Coverage   73.81%   73.71%   -0.10%     
==========================================
  Files          18       18              
  Lines        5423     5449      +26     
==========================================
+ Hits         4003     4017      +14     
- Misses       1420     1432      +12     
Files with missing lines Coverage Δ
python/simulation.py 77.41% <83.33%> (+<0.01%) ⬆️

@oskooi
Copy link
Collaborator

oskooi commented Aug 30, 2024

Currently, my entire simulation of Meep runs in a Simulation class and I would like to keep my step functions in this class as well. That being said, they must have self as an arg. With the current _eval_step_func, the arg handler is a bit too strict.

If you haven't already done so, it might be useful to first read The Run Function is not a Loop which provides a description of the step-function object including examples.

@stevengj
Copy link
Collaborator

stevengj commented Sep 3, 2024

Note that you can already pass as many additional arguments you want to the step function by wrapping it in a closure.

For example, if you have a function my_step(a,b,c,sim), you can pass it as a step function with lambda sim: my_step(a,b,c,sim), in which the additional arguments a,b,c are captured by the closure.

Or, if you have a function foo.my_step(sim) that is a bound method of some object foo, you can pass it as a step function with lambda sim: foo.my_step(sim).

@stevengj
Copy link
Collaborator

stevengj commented Sep 3, 2024

Overall, I'm in favor of something like this to make it easier to pass bound methods for step functions.

However, what I'd like to do is to simply fix get_num_args to subtract 1 if the function is a bound method, as determined by inspect.ismethod(func) (not from the argument names). That is, get_num_args should return the number of non-self arguments.

@stevengj stevengj merged commit 1f77440 into NanoComp:master Sep 5, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants