Skip to content

Fixes for MTK v8 #26

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

Merged
merged 91 commits into from
Apr 23, 2022
Merged

Conversation

ValentinKaisermayer
Copy link
Contributor

@ValentinKaisermayer ValentinKaisermayer commented Apr 11, 2022

Changes

  • Blocks make use of a connector in the form of a RealInput and RealOutput (as Modelica does). However, this could be improved since there is only one variable in the connector. Definitions of input=true and output=true where removed since this feature is not fully developed in MTK. Can be added later in one place.
  • Where applicable Electrical components make use of OnePort.
  • Where applicable Thermal components make use of Element1D.

Still open

  • Tests for Magnetic
  • PID-controller and StateSpace block
  • A bunch of math blocks e.g. Tan
  • A bunch of source blocks e.g. Square
  • Doc strings
  • Port names, parameter names and component names should be as in the Modelica standard library. The goal would be that existing Modelica examples can be almost copy pasted.

@ChrisRackauckas
Copy link
Member

Can we get to a merge point and then do a different PR to update the other things? I think too many PRs in this repo have been big "fix all of the things" PRs that never got merged, that then stopped other people from contributing as they waited for someone to finish a "fix all of the things" PR. So a "here's MTK v8" with 7 issues of harder things to update, that's a much better option in my book IMO, unless you plan to really knock out that list in a week.

Co-authored-by: Fredrik Bagge Carlson <[email protected]>
@ChrisRackauckas
Copy link
Member

Deleting the tests is definitely a bad thing. We won't be able to merge this with those older tests removed.

@ValentinKaisermayer
Copy link
Contributor Author

Deleting the tests is definitely a bad thing. We won't be able to merge this with those older tests removed.

I re-added explicit tests for the sim results of the analog sources, first and second order systems. I also added them for the block sources. I hope this will be sufficient.

I also added a first test for the magnetic domain. However, most of the magnetic components are still not covered.

@ChrisRackauckas
Copy link
Member

The only tests I see are missing are the digital tests. Those are commented out. With those back I think this is ready to merge.

@ValentinKaisermayer
Copy link
Contributor Author

The only tests I see are missing are the digital tests. Those are commented out. With those back I think this is ready to merge.

They are commented out in the current release as well.

@ChrisRackauckas
Copy link
Member

Okay cool, amazing. Just noticed that. I think it's fine to merge this and call it v1.0, and further improvements will continue to happen, but this is a major step forward already and at least 1000x better than what we had before. Major props to @ValentinKaisermayer !

@ChrisRackauckas ChrisRackauckas merged commit 7801a39 into SciML:main Apr 23, 2022
@ValentinKaisermayer ValentinKaisermayer deleted the fix-MTK-v8 branch April 23, 2022 15:21
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