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

Feature/451 ssp linear transformations #454

Merged
merged 17 commits into from
Nov 7, 2019

Conversation

markaren
Copy link
Contributor

@markaren markaren commented Oct 31, 2019

Closes #451

Should add test, but want to hear feedback first.

@kyllingstad
Copy link
Member

I like it!

I'd prefer we avoid protected member variables, though, so my recommendation would be to implement linear_transformation_connection::set_source_value() in terms of scalar_connection::set_source_value() instead of accessing scalar_connection::value_ directly.

Copy link
Member

@eidekrist eidekrist left a comment

Choose a reason for hiding this comment

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

Nice!

@markaren
Copy link
Contributor Author

markaren commented Nov 6, 2019

I take it that the positive feedback means we agree on this approach?

Add test, and then we have an approval?

@kyllingstad
Copy link
Member

Should we require that all features that we implement for SSP must also be implemented for OspSystemStructure? Since we have defined the latter as our primary format, it would perhaps seem strange if the SSP support wasn't a subset of that.

@markaren
Copy link
Contributor Author

markaren commented Nov 6, 2019

Should we require that all features that we implement for SSP must also be implemented for OspSystemStructure

I think this is to strict. What if OspSystemStructure strays way off compared to SSP, then it may be very difficult to support potential useful fetatures/changes defined in e.g. SSP 2.0.

However, this particular feature should definitively be supported in both. But not necessarily in the same PR.

If it was up to me though, OspSystemStructure would be a superset of SSP. Like YAML to JSON.

@markaren
Copy link
Contributor Author

markaren commented Nov 6, 2019

If it was up to me though, OspSystemStructure would be a superset of SSP. Like YAML to JSON.

Chew on that a little bit. That would in practice mean that we could use the same parser for both formats.

@eidekrist
Copy link
Member

I'm happy with approving this once a test is in place. This feature should definitely be available for OspSystemStructure as well, so I have created #465.

On the topic of OspSystemStructure as a superset of SSP, that's a clear no from me for the time being. Might be worth revisiting after the project is concluded.

@eidekrist
Copy link
Member

@kyllingstad: Should we require that all features that we implement for SSP must also be implemented for OspSystemStructure? Since we have defined the latter as our primary format, it would perhaps seem strange if the SSP support wasn't a subset of that.

Agreed. We don't have that much time left in this project, and I would even say that OspSystemStructure should always come first, and SSP at best second.

@markaren
Copy link
Contributor Author

markaren commented Nov 6, 2019

Unless SSP is abandoned, I reckon it will continue to be favored from mine and NTNUs side.

Copy link
Member

@eidekrist eidekrist left a comment

Choose a reason for hiding this comment

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

Looks OK!

test/cpp/ssp_linear_transformation_unittest.cpp Outdated Show resolved Hide resolved
@markaren markaren merged commit bc23fd4 into master Nov 7, 2019
@eidekrist
Copy link
Member

Next time please wait for approval before merging.

@markaren
Copy link
Contributor Author

markaren commented Nov 7, 2019

I took your last comment as an approval. Sorry about that.

@markaren markaren deleted the feature/451-ssp-linear-transformations branch November 7, 2019 12:25
double offset = 50;
double factor = 1.3;
double target = factor * initialValue + offset;
BOOST_REQUIRE_CLOSE(transformedValue, target, 1e-9);
Copy link
Member

Choose a reason for hiding this comment

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

Might be interesting for future usage/debugging: The tolerance parameter (1e-9 here) represents a percentage value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Auch. Well, we'll fix that. What would a decent value here be?

Copy link
Contributor Author

@markaren markaren Nov 7, 2019

Choose a reason for hiding this comment

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

Might be interesting for future usage/debugging

Nice thing about Boost.test vs the others tests is that the what and where is shown, so it would not have been difficult to see what happened.
Anyway fixed in: #472

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.

[SSP] Support linear transformations
3 participants