search

muralidn / CAS741-Fall20

Repository for the class CAS-741: Development of Scientific Computing Software. See the README.md for details about the project.
Other
1 stars 0 forks source link

Drasil SRS review #56

Open muralidn opened 4 years ago

muralidn commented 4 years ago

@smiths

Please review the first draft of SRS generated with Drasil found here,

https://github.com/muralidn/Drasil/tree/PIDController/code/stable/pidController

I will be fixing "known" issues over the next couple of days.

Thank you!

smiths commented 4 years ago

@muralidn, please include a pointer to your Drasil code as well.

muralidn commented 4 years ago

Thanks Dr. @smiths.

Here is the Drasil source code,

https://github.com/muralidn/Drasil/tree/PIDController/code/drasil-example/Drasil/PIDController

I did not get a chance to figure out 2nd order ODE solver yet, I will be doing that this week.

smiths commented 4 years ago

@muralidn, I've reviewed your Drasil generated SRS. The comments are on a marked up pdf in 42b49b6.

I also looked at the Drasil code, but I don't have any comments on what you did. It looks good to me, although looking over the code does highlight some "bigger picture" issues with Drasil. As two examples of future improvements to Drasil:

  1. You define the people for your project in the same place as the people for every one of the existing Drasil projects. It seems like there should be a way to define people that are local to a specific project.

  2. I don't like that the information on the ODE is entered twice - once for the IM and once for the code generator. It is basically the same equation twice. That is the kind of thing that Drasil is intended to avoid! :smile:

muralidn commented 4 years ago

Thank You Dr. @smiths !

Regarding point 1, the author was defined in the Drasil as per the instructions in the Drasil User's guide. However, a Person type can be created locally in the project as well. I have updated my project accordingly with commit id, https://github.com/muralidn/Drasil/commit/42e585f73be4132d295b0c10e1e6f57acbd33b4f.

Point 2 is a limitation at the moment. Ideally, ODEInfo must be integrated into the InstanceModeltype.

smiths commented 4 years ago

Thanks for the feedback @muralidn.