Skip to content

Conversation

@vitorhirata
Copy link
Contributor

This PR fixes two errors on the 'Bare node creation' test of IHACRESExpuhNode

  1. Running this test was causing the error MethodError: no method matching unsafe_convert(::Type{Float64}, ::Int64). This happened because tau_s value was an integer and should be a float. The PR changes the default value of tau_s from 5 to 5.0.
  2. Running this test was causing the error MethodError: no method matching update_state. This is happening because the commit 775a575 changed the definition of some methods, and not all usage of these methods has changed. The PR changes two calls to update_state to update_state!.

@ConnectedSystems let me know if this makes sense. I went straight to creating a PR and not an issue because it seemed like a simple change, but let me know if you'd prefer something else.

The 'Bare node creation' test was returning an error when passing an
expuh: `MethodError: no method matching unsafe_convert(::Type{Float64},
::Int64)`. This happened because tau_s value was an integer and should
be a float. This commit change the default value of tau_s from 5 to 5.0.
The "Bare node creation" test using IHACRESExpuhNode is returning an
error: `MethodError: no method matching update_state`. This is happening
because the commit 775a575 changed the
definition of some methods and not all usage of them have changed. It
adds a "!" to the update_state method on IHACRESExpuhNode.jl.
@ConnectedSystems
Copy link
Owner

Hi @vitorhirata

Thanks for this. Yes, if you already have a fix in hand, I prefer a PR be submitted directly rather than an issue and a PR. Otherwise, it splits the discussion into two locations.

I have a number of big changes planned which is why I wasn't fussed about tests failing at this time. One of which may be to restructure IHACRES so the different formulations are more modular.

The changes in this PR look fine in any case, so I'll merge when CI finishes.

@ConnectedSystems ConnectedSystems merged commit f0637da into ConnectedSystems:main Dec 13, 2024
0 of 4 checks passed
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.

2 participants