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

fixing issue #31 with updated node docs #46 #47

Merged
merged 5 commits into from
Feb 27, 2019
Merged

fixing issue #31 with updated node docs #46 #47

merged 5 commits into from
Feb 27, 2019

Conversation

SabineAuer
Copy link
Member

To fix issue #31 I changed the docs for the FourthEq-model [1] by adding the explanation for the inertia-normalized frequency. Also I added the references for the inverter models for VSIMinimal [2] and VSIVoltagePT1 model [3]

* [1] https://juliaenergy.github.io/PowerDynamics.jl/stable/node_types.html#PowerDynBase.FourthEq

* [2] https://juliaenergy.github.io/PowerDynamics.jl/stable/node_types.html#PowerDynBase.VSIMinimal

* [3] https://juliaenergy.github.io/PowerDynamics.jl/stable/node_types.html#PowerDynBase.VSIVoltagePT1

@timziebart
Copy link
Member

  1. Please build the docs and read through it before submitting. You'll see a few things that are not correct.
  2. Also, as there are quite a lot of equations it might be good to give a short (1 sentence is enough) explantion of what each bunch of the equations actually does. The idea of the docs is that people can read it and quickly understand what is being done (assuming they have certain background in power dynamics modeling).
  3. Furthermore, there are quite a few unexplained dynamic variables.
  4. We have conventions on how to call write parameters, please follow them.

- `X_q_dash`: transient reactance of q-axis
- `X_d`: reactance of d-axis
- `X_d`: reactance of q-axis
- `Ω_H`: normalized `Ω` (given in [``s^{-2}``]) according to `Ω_H = (Ω⋅2\pi) / H`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not a Keyword argument, right? It's just a temporary variable computed in the code. It should be removed here. And I don't actually think there should be any further documentation of this variable except what is already directly in the code.

@timziebart
Copy link
Member

timziebart commented Feb 13, 2019

Except for the one line that I just reviewed, I think that looks very good. Could you simply remove that single line?

Now, I would suggest we fix the ci problems with travis #49, then fix the conflicts with the (then) master branch and then squash'n'merge. Do you agree, @SabineAuer ?

@SabineAuer
Copy link
Member Author

That's fine for me. I'll remove the line and recommit to the branch. I agree that the fix of the travis CI should be high prio from now on.

timziebart
timziebart previously approved these changes Feb 14, 2019
@SabineAuer
Copy link
Member Author

SabineAuer commented Feb 26, 2019

@timkittel: I merged the master into the branch of this PR so the travis CI can run through for this PR as well. Now all checks but the code coverage succeeded. The latter says:

Showing 1 files with coverage changes found.

I am not sure what to do with this failed check. Can you help me out?

@timziebart
Copy link
Member

Some how coverage seems to have gone down by one line. I am not sure, why that happens, but in this case I would say it's okai. Improving coverage is an issue anyway.

So from my point of view, you can force merge it.

@timziebart
Copy link
Member

(Probably the total number of lines has gone down, which makes the absolute number of uncovered lines relatively higher.)

@SabineAuer SabineAuer merged commit 39d21af into master Feb 27, 2019
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.

None yet

2 participants