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

Clarification on model ACEc and ACE #586

Merged
merged 3 commits into from
Dec 13, 2024
Merged

Conversation

jinningwang
Copy link
Member

#585

  • Fix ACEc.bias.tex_name
  • Change ACEc.bias.default to -1 to follow NERC's definition
  • Add references

@jinningwang
Copy link
Member Author

@cuihantao
One question regarding this PR.

As here I changed the default value and equation, it seems to be a potential breaking change. Before manually updating parameters of ACEc.bias, users can run into unexpected results.

So, I'd like your suggesstion that which option would be better for us?

  1. Revise the equations: use abs() to eliminate the impact of sign
  2. If there is a parameter correction step, correct positive values to negative

@cuihantao
Copy link
Collaborator

If it's always going to be a negative value, you can use "abs". This is to safeguard users who entered the value mistakenly as a positive value.

If both positive and negative values have a meaning, then we should do any correction to parameter but rather correct the equation.

@jinningwang
Copy link
Member Author

Bias and frequency response are both expressed as negative numbers. In other words, as frequency drops, MW output (β) or desired output (B) increases. Both are measured in MW/0.1 Hz

Given NERC's suggestion, I update the PR with first option.

@jinningwang
Copy link
Member Author

@cuihantao ready to merge

@cuihantao cuihantao merged commit e442d38 into CURENT:develop Dec 13, 2024
3 checks passed
@cuihantao
Copy link
Collaborator

Jinning, thanks!! I'll tag a new version shortly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants