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

Add a Delay model component #106

Merged
merged 1 commit into from
Mar 30, 2021
Merged

Add a Delay model component #106

merged 1 commit into from
Mar 30, 2021

Conversation

taldcroft
Copy link
Member

Description

Add a new model component class Delay to delay the compute model values from node by delay ksec.

For a positive delay, the computed model value (node.mval) will be constant at the initial value for the first delay ksec. Conversely for a negative delay the values at the end will be constant for delay ksec.

Testing

  • Passes unit tests on MacOS
  • Functional testing

Functional testing

Made a new thermal model for 4HFSPAT using the Delay component (now included in the examples/delay directory).

  • Confirmed that the model component introduces the expected positive or negative delay, with the endpoints being fixed as expected.
  • Confirmed that the delay parameter can be successfully fit with xija_gui_fit and that the result is reasonable (i.e. the model and data peaks visually line up).

Copy link
Collaborator

@jzuhone jzuhone left a comment

Choose a reason for hiding this comment

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

Can you remind me what the instability in xija_gui_fit is? And should we fix it?

@taldcroft
Copy link
Member Author

@jzuhone - the instability was just related to this new Delay component if the code was

comp.node.mvals[:] = np.interp(x=self.times - comp.delay * 1000,
                                                xp=self.times, fp=comp.node.mvals)

instead of:

comp.node.mvals[1:] = np.interp(x=self.times - comp.delay * 1000,
                                                xp=self.times, fp=comp.node.mvals)[1:]

This is what I showed at the TWG when I moved the delay slider left and right and the beginning of the model calculation got wacky. This does not happen for the current code using [1:] to avoid copying the first element of the array.

Copy link
Contributor

@matthewdahmer matthewdahmer left a comment

Choose a reason for hiding this comment

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

I generated two variants of the pline04t model that had a delay and a pseudo node, and one that had the delay with no pseudo node. Both models seemed to run as expected in gui fit.

@taldcroft taldcroft merged commit cd65bf9 into master Mar 30, 2021
@taldcroft taldcroft deleted the delay-component branch March 30, 2021 13:24
@javierggt javierggt mentioned this pull request Jul 2, 2021
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.

3 participants