Skip to content

Add Electrical Component docstrings #7

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

Closed
wants to merge 8 commits into from

Conversation

AayushSabharwal
Copy link
Member

@AayushSabharwal AayushSabharwal commented Nov 5, 2021

Working on adding some docstrings, trying to follow the pattern in MTK.jl docs.

Some questions:

  • Do the ModelingToolkit.connect methods need docstrings? Consequently, does ElectricalPin need one?
  • Do the ideal components need to specify how their variables are calculated?
  • I've studied OpAmps a bit, but I'm not particularly sure how this one works
  • How should the pins of sensors (and components in general) be documented? All two-pin components have pins p and n, so should this be mentioned in every sensor/component or left as an assumed standard? What about components with more pins?

EDIT: I'm not sure why I didn't just take the docs from the README

Added docstrings for:
- Pins
- Ideal components
- Sensors
- Docstrings are more in line with README
- Pins mentioned in Connectors section
- Variables section renamed to States
- Parameters mentioned in Observables section
- Updated OpAmp docstring
@AayushSabharwal AayushSabharwal mentioned this pull request Nov 7, 2021
@AayushSabharwal
Copy link
Member Author

It seems like documenting macro usages is not straightforward. The docstrings for @connector function Pin and @connector function DigitalPin seem to be the issue. I did come across this but it doesn't seem to work here, just leads to another error in MTK.jl

@AayushSabharwal
Copy link
Member Author

Apologies for the inactivity. I have a lot going on right now. Would love to get back to this in a week!

- `ConstantVoltage`, `CosineVoltage` documented
- `Pin` and `DigitalPin` documented (using `@doc`)
@AayushSabharwal
Copy link
Member Author

Documenting Pin and DigitalPin requires @doc to be used, since they are the result of macro evaluations

- Docstrings for `DampedSineVoltage`, `RampVoltage`, `SineVoltage`, `SquareVoltage`
@AayushSabharwal
Copy link
Member Author

If #18 is on its way to completion, it might be better to wait and migrate the docstrings over to those new functions? It would reduce a lot of duplicacy

- Added docstrings for Analog `StepVoltage`, `TriangularVoltage` components
- Added docstrings for all Digital components
@AayushSabharwal
Copy link
Member Author

I'm not familiar with PulseDiff, but the rest of the docstrings are done. I left out the current sources because of the above comment. They would be nearly identical to the docstrings of the corresponding voltage sources.

@AayushSabharwal AayushSabharwal marked this pull request as ready for review December 10, 2021 08:36
@ChrisRackauckas
Copy link
Member

Looks good thanks!

@ChrisRackauckas
Copy link
Member

Test failure

@AayushSabharwal
Copy link
Member Author

I'll look into this. Weird that docs failed tests.

@AayushSabharwal
Copy link
Member Author

AayushSabharwal commented Dec 12, 2021

It seems that @register _and(x...) is the issue, in src/Electrical/Digital/tables.jl:59. Symbolics deprecated @register for @register_symbolic, and it doesn't like that. This happens on the main branch too.

I tried updating to using @register_symbolic, but that doesn't fix it either and would break compatibility with pre-v4 Symbolics.

@ChrisRackauckas
Copy link
Member

we should just drop the earlier Symbolics and update.

@AayushSabharwal
Copy link
Member Author

I'm afraid this still won't precompile, because Symbolics doesn't like the x... in @register_symbolic _and(x...)

@AayushSabharwal
Copy link
Member Author

AayushSabharwal commented Dec 13, 2021

It seems the deprecation of @register for @register_symbolic broke this. If I use @register everywhere on Symbolics v4.1.0 this works fine

v4.1.1 breaks this, so from looking at the release log, my guess would be that JuliaSymbolics/Symbolics.jl#457 is the relevant PR causing issues

@AayushSabharwal
Copy link
Member Author

AayushSabharwal commented Dec 23, 2021

MTK v8 removed promote_connect_rule in SciML/ModelingToolkit.jl#1348, which breaks this. What would be the new way to support this?

@ChrisRackauckas
Copy link
Member

If the variables are appropriately defined as flow variables this should all happen automatically through the connect function.

@AayushSabharwal
Copy link
Member Author

AayushSabharwal commented Dec 23, 2021

I guess that should happen in #25 then. Symbolics v4.1.1 still breaks this, and main as well.

@AayushSabharwal
Copy link
Member Author

Tests should pass now with Symbolics v4.2.2 release

@ChrisRackauckas
Copy link
Member

Please update this to match master. We just had a pretty major change (#7) and it would be good to copy these over if possible.

@ChrisRackauckas
Copy link
Member

Looking at http://mtkstdlib.sciml.ai/dev/API/electrical/, I like how your format defines the ports and everything, so updating it would be great.

@AayushSabharwal
Copy link
Member Author

Sure! I'm a bit swamped with work right now, but I'll get to this as soon as I can.

@ChrisRackauckas
Copy link
Member

Oh no, don't merge, rebase this one 😓. You might want to just start from scratch and copy over the major changes.

@AayushSabharwal
Copy link
Member Author

I guess that's a better idea. I spent the past half hour trying to reconcile all of this only for it to show up again :P

Rebasing was annoying, since it seemed to process commit by commit and kept getting merge conflicts in the same files at the same places repeatedly, so I tried --no-rebase. Looks like git still doesn't like how this branch looks. I'll make another one and PR again.

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.

2 participants