Skip to content

Conversation

@MichaelClerx
Copy link
Member

Questions:

  • Why is there a plot argument? Setting it to True gives an error if no ax or fig path is set.
  • Can we stick to a single order for t, v, i throughout? This one is especially annoying as it goes i, t, segment_info, v

@MichaelClerx
Copy link
Member Author

Should also be using detect_ramp_bounds instead of replicating that code?

@MichaelClerx MichaelClerx marked this pull request as draft October 16, 2025 12:39
@joeyshuttleworth
Copy link
Collaborator

These suggestions sound very sensible.

  • "Plot" flag is bugged , but I don't think has been exposed because we must be setting it to True all the time?
  • Happy to reorder the parameters. We should do a separate pull request for that and choose one ordering
  • Detect ramp bounds should work here, you need to pick the last ramp, i.e. ramp_no=-1. It's not immediately obvious what this one-liner is doing without being familiar with how voltage_segments is setup. So this change would make it much more readable. We also would have an exception being thrown if there isn't a ramp (definitely could happen if you use some other protocol).

A bit tangentially, maybe "ramp_no" isn't the best parameter name for detect_ramp_bounds. Perhaps "ramp_index" is clearer

@joeyshuttleworth
Copy link
Collaborator

Plotting options seem a bit confused in the original code. I think I got confused in trying to give a few options: allowing the user to provide an axis, allowing the user to save a plot to disc without providing an axis, and not plotting anything at all. It might be simpler to only deal with an "ax" parameter, and not deal with any figure creation.

In any case, the plot parameter isn't really needed, and we should just get rid of it.

@MichaelClerx MichaelClerx marked this pull request as ready for review October 16, 2025 13:38
@MichaelClerx
Copy link
Member Author

@joeyshuttleworth OK to merge this?

@MichaelClerx MichaelClerx merged commit 2db0772 into main Oct 17, 2025
8 checks passed
@MichaelClerx MichaelClerx deleted the infer-e-docstring branch October 17, 2025 12:42
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.

3 participants