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

Optional ID structural refactor #828

Merged
merged 38 commits into from
Nov 25, 2024
Merged

Conversation

figueroa1395
Copy link
Contributor

@figueroa1395 figueroa1395 commented Nov 11, 2024

Implements #806 and continues the refactor started in #824. The scope of this PR is as follows:

  • Remove hard-coded -1s in main_model_impl.hpp. 9810322
  • Minor renaming. 56534a8
  • Refactor check_components_independence. e7e734c
  • Rename update_component overload for multiple components. 9cdb0d6
  • Move and refactor independence related functionality to update.hpp. The rest of commits up until 2143cba address this.
  • Final re-organize. cc76897

Out of scope:

  • Move update_component out of main_model_impl.hpp.
  • Reduce update_component overload number.
  • Variadic template with V suffix.
  • Homogenize CT, CompType and ComponentType.

Signed-off-by: Santiago Figueroa Manrique <[email protected]>
Signed-off-by: Santiago Figueroa Manrique <[email protected]>
Signed-off-by: Santiago Figueroa Manrique <[email protected]>
Signed-off-by: Santiago Figueroa Manrique <[email protected]>
Signed-off-by: Santiago Figueroa Manrique <[email protected]>
Signed-off-by: Santiago Figueroa Manrique <[email protected]>
Signed-off-by: Santiago Figueroa Manrique <[email protected]>
Signed-off-by: Santiago Figueroa Manrique <[email protected]>
Signed-off-by: Santiago Figueroa Manrique <[email protected]>
@figueroa1395 figueroa1395 added the improvement Improvement on internal implementation label Nov 13, 2024
@figueroa1395 figueroa1395 self-assigned this Nov 13, 2024
Signed-off-by: Santiago Figueroa Manrique <[email protected]>
…refactor-part2

Signed-off-by: Santiago Figueroa Manrique <[email protected]>
Signed-off-by: Santiago Figueroa Manrique <[email protected]>
Base automatically changed from feature/refactor-optional-id to main November 13, 2024 13:00
Signed-off-by: Santiago Figueroa Manrique <[email protected]>
Signed-off-by: Santiago Figueroa Manrique <[email protected]>
Signed-off-by: Santiago Figueroa Manrique <[email protected]>
Signed-off-by: Santiago Figueroa Manrique <[email protected]>
Signed-off-by: Santiago Figueroa Manrique <[email protected]>
Signed-off-by: Santiago Figueroa Manrique <[email protected]>
Signed-off-by: Santiago Figueroa Manrique <[email protected]>
@figueroa1395 figueroa1395 marked this pull request as ready for review November 15, 2024 15:25
@figueroa1395
Copy link
Contributor Author

I just realized that check_id_na was supposed to be moved to dataset.hpp. I will address that on Monday, but besides that, everything else is ready.

@Jerry-Jinfeng-Guo
Copy link
Contributor

you don't, that's my point. at some point, the scope is "large enough" and you should focus on what's currently already in scope. anything new that pops up can be moved to a follow-up (even if it's only minor). if there are no follow-ups: great

This is self contradictory statement.

@mgovers
Copy link
Member

mgovers commented Nov 21, 2024

you don't, that's my point. at some point, the scope is "large enough" and you should focus on what's currently already in scope. anything new that pops up can be moved to a follow-up (even if it's only minor). if there are no follow-ups: great

This is self contradictory statement.

which part?

Signed-off-by: Santiago Figueroa Manrique <[email protected]>
@Jerry-Jinfeng-Guo
Copy link
Contributor

you don't, that's my point. at some point, the scope is "large enough" and you should focus on what's currently already in scope. anything new that pops up can be moved to a follow-up (even if it's only minor). if there are no follow-ups: great

This is self contradictory statement.

which part?

"anything new that pops up can be moved to a follow-up (even if it's only minor)" vs "if there are no follow-ups: great"
My original question is tied to this PR, not a theoretical question. You agreed to scope this PR, sure, no problem, on the condition the mentioned things will be addressed in a follow-up, which is the premise of the whole discussion.

Copy link
Member

@mgovers mgovers left a comment

Choose a reason for hiding this comment

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

huge improvement. a couple remarks but nothing serious. feel free to resolve them

@figueroa1395 figueroa1395 changed the title Feature/Optional ID structural refactor Optional ID structural refactor Nov 22, 2024
Signed-off-by: Santiago Figueroa Manrique <[email protected]>
Signed-off-by: Santiago Figueroa Manrique <[email protected]>
@figueroa1395
Copy link
Contributor Author

figueroa1395 commented Nov 22, 2024

c56adb2 Still not finished as now there are some memory leaks. Either it is something very "simple" I missed, or something very complex and then we may want a follow up to separate from this already large PR.

The issue may be related with independence_flags in main_model_impl.hpp.

@TonyXiang8787
Copy link
Member

@mgovers, @figueroa1395, @Jerry-Jinfeng-Guo

I see the PR was initially green but after some commits it turns to red.

Maybe we can revert some refactors and merge this?

@mgovers
Copy link
Member

mgovers commented Nov 25, 2024

@mgovers, @figueroa1395, @Jerry-Jinfeng-Guo

I see the PR was initially green but after some commits it turns to red.

Maybe we can revert some refactors and merge this?

i think so as well. it's very easy to revert the last 2 commits and then reapply in a new PR or cherry-pick the changes.

@Jerry-Jinfeng-Guo
Copy link
Contributor

@mgovers, @figueroa1395, @Jerry-Jinfeng-Guo
I see the PR was initially green but after some commits it turns to red.
Maybe we can revert some refactors and merge this?

i think so as well. it's very easy to revert the last 2 commits and then reapply in a new PR or cherry-pick the changes.

Let's indeed make the additional commits a follow-up PR and merge this one for now.

This reverts commit c56adb2.

Signed-off-by: Santiago Figueroa Manrique <[email protected]>
@figueroa1395
Copy link
Contributor Author

@mgovers, @figueroa1395, @Jerry-Jinfeng-Guo

I see the PR was initially green but after some commits it turns to red.

Maybe we can revert some refactors and merge this?

Reverted the last "problematic" commit. Will merge after CI passes and then open another PR.

@figueroa1395 figueroa1395 added the do-not-merge This should not be merged label Nov 25, 2024
@figueroa1395
Copy link
Contributor Author

Merge only after #833 is merged, just for safety. That is why I added the do-not-merge label.

@mgovers mgovers removed the do-not-merge This should not be merged label Nov 25, 2024
Copy link

sonarcloud bot commented Nov 25, 2024

@figueroa1395 figueroa1395 added this pull request to the merge queue Nov 25, 2024
Merged via the queue into main with commit 02f9fd4 Nov 25, 2024
27 checks passed
@figueroa1395 figueroa1395 deleted the feature/optional-id-refactor-part2 branch November 25, 2024 12:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improvement on internal implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants