Skip to content

[fix] modm::Saturated - #780

Merged
salkinium merged 1 commit into
modm-io:developfrom
TomSaw:fix/math-saturation
Dec 14, 2021
Merged

[fix] modm::Saturated#780
salkinium merged 1 commit into
modm-io:developfrom
TomSaw:fix/math-saturation

Conversation

@TomSaw

@TomSaw TomSaw commented Nov 15, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

To PR the generic colors nextly from #665 , i would like to PR this fixed modm::Saturated first.

TODO

  • Add missing operator+= / -=
  • Extend tests to more types and more cases

@TomSaw
TomSaw force-pushed the fix/math-saturation branch from 473cefa to 6c25b15 Compare November 15, 2021 12:07
@TomSaw TomSaw changed the title fixed modm::Saturated [fixed modm::Saturated Nov 15, 2021
@TomSaw TomSaw changed the title [fixed modm::Saturated [fix] modm::Saturated Nov 15, 2021
@TomSaw
TomSaw force-pushed the fix/math-saturation branch from 6c25b15 to 9d0c642 Compare November 15, 2021 12:09
@TomSaw TomSaw mentioned this pull request Nov 15, 2021
Open
4 tasks
salkinium
salkinium reviewed Nov 15, 2021
View reviewed changes

@salkinium salkinium left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice!

Comment thread src/modm/math/saturation/saturated.hpp Outdated
Comment thread src/modm/math/utils/integer_traits.hpp Outdated
Comment thread src/modm/math/utils/integer_traits.hpp Outdated
Comment thread src/modm/math/saturation/saturated.hpp Outdated
@TomSaw
TomSaw force-pushed the fix/math-saturation branch 2 times, most recently from c358128 to 8bfb47a Compare November 23, 2021 08:07
@TomSaw
TomSaw force-pushed the fix/math-saturation branch from 8bfb47a to d21beb8 Compare December 12, 2021 11:05
@TomSaw

TomSaw commented Dec 12, 2021

Copy link
Copy Markdown
Contributor Author

Done! Please review @salkinium

salkinium
salkinium approved these changes Dec 12, 2021
View reviewed changes

@salkinium salkinium left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Lovely!

@TomSaw
TomSaw force-pushed the fix/math-saturation branch 16 times, most recently from fba61f5 to c9d4d44 Compare December 13, 2021 07:15
@TomSaw

TomSaw commented Dec 13, 2021

Copy link
Copy Markdown
Contributor Author

Could be merged @salkinium 😋

@TomSaw

TomSaw commented Dec 13, 2021

Copy link
Copy Markdown
Contributor Author

... still in exams phase?

@salkinium

salkinium commented Dec 13, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

... still in exams phase?

Way worse: master thesis phase for the next ~6 months.

@salkinium

salkinium commented Dec 13, 2021

Copy link
Copy Markdown
Member

Could be merged @salkinium 😋

Rebase and squash?

@TomSaw
TomSaw force-pushed the fix/math-saturation branch from c9d4d44 to 64db86f Compare December 13, 2021 20:55
@TomSaw

TomSaw commented Dec 13, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Way worse: master thesis phase for the next ~6 months.

So i stay in @-poke-mode for ~6 months @salkinium 😬 !? Hope you find a breath in between.

@salkinium

salkinium commented Dec 14, 2021

Copy link
Copy Markdown
Member

So i stay in @-poke-mode for ~6 months @salkinium 😬 !?

You should probably always stay in poke mode, since you push so many commits that I don't really know when do reviews or merges 😜 So it's best if you just tell me, that helps me and you.

@TomSaw
TomSaw force-pushed the fix/math-saturation branch from 53a15f6 to c0a8c51 Compare December 14, 2021 09:02
@salkinium
salkinium merged commit c0a8c51 into modm-io:develop Dec 14, 2021
@TomSaw

TomSaw commented Dec 14, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Thanks for the feedback, i thought like pushing commits that hard isn't normal. I rely to much on githubs CI and it always surprises me - repeating - what else got broken with my changes.

I can run anything within '/test' but there is more! Running githubs complete CI locally would save a ton of nerves and CO2. Is there some 'best practice' to read? @salkinium

@salkinium

salkinium commented Dec 14, 2021

Copy link
Copy Markdown
Member

See this Readme here: https://github.com/modm-io/modm/tree/develop/test#unit-tests

@salkinium

salkinium commented Dec 14, 2021

Copy link
Copy Markdown
Member

There are also a bunch of scripts in tools/scripts, for the usage consult the GitHub CI runner: https://github.com/modm-io/modm/blob/develop/.github/workflows/linux.yml#L32-L62

@salkinium

salkinium commented Dec 14, 2021

Copy link
Copy Markdown
Member

The compile-all tests take a very long time locally, I suggest only running them only locally for a subset of devices if the ci:hal label has failed.
By calling it manually you can filter the failing devices by their prefix python3 run_all.py $PREFIX which reduced the scope significantly.
There are also ignored devices, but they are centrally handled by repo.lb: https://github.com/modm-io/modm/blob/develop/test/all/ignored.txt

@TomSaw
TomSaw deleted the fix/math-saturation branch December 14, 2021 16:32
@salkinium salkinium added this to the 2021q4 milestone Dec 18, 2021
@TomSaw

TomSaw commented Jan 11, 2022

Copy link
Copy Markdown
Contributor Author

Perfect. Thank you!

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

Reviewers

@salkinium salkinium salkinium approved these changes

Assignees

No one assigned

Labels

enhancement 🌈 feature 🚧

Milestone

2021q4

Development

Successfully merging this pull request may close these issues.

2 participants

@TomSaw @salkinium