Skip to content

Removed dimension field from Cal3#2178

Merged
dellaert merged 4 commits intoborglab:developfrom
CodeXTL:fix/removed-dim-from-Cal3
Jun 28, 2025
Merged

Removed dimension field from Cal3#2178
dellaert merged 4 commits intoborglab:developfrom
CodeXTL:fix/removed-dim-from-Cal3

Conversation

@CodeXTL
Copy link
Copy Markdown
Contributor

@CodeXTL CodeXTL commented Jun 25, 2025

Primary Change(s):

  • Removed dimension field and Dim() from Cal3.h
  • Made dim() a pure virtual function in Cal3.h. This made Cal3 an abstract class. This was done rather than removing the function entirely because derived classes override this function.
  • Added an implementation of dim() with override keyword so that Cal3_S2 becomes a concrete class. Not doing so made it abstract, which caused some errors in the unit tests

Check(s) Done:

  • Executed make check, 100% tests passed

Note(s):

  • This is a small PR as I have decided to divide my work based on relevance in order to prevent a backlog of PRs.

@CodeXTL
Copy link
Copy Markdown
Contributor Author

CodeXTL commented Jun 25, 2025

I believe I have realized what's causing the Python CI checks to fail; fixing it right now

@dellaert
Copy link
Copy Markdown
Member

Thanks! Two comments:

  • I think dim() should also be removed from Cal3. What test fails if you remove it?
  • then you don’t have to remove the Cal3 constructors, either.

@CodeXTL
Copy link
Copy Markdown
Contributor Author

CodeXTL commented Jun 26, 2025

@dellaert Responses for comments:

  • All the derived classes override dim() from Cal3 at the moment. I could remove dim() from Cal3 if that's preferred, and the only change I would need to do is remove all the override keyword from the derived classes' definition of dim()
  • Is there a reason why we want to keep the wrappers for the constructors given that Cal3 is an abstract base class? Would there ever exist a reason for a user to create an instance of Cal3?

@dellaert
Copy link
Copy Markdown
Member

So:

  • yes, please remove dim()
  • Then it is no more an abstract base class :-) so keep constructors

@CodeXTL
Copy link
Copy Markdown
Contributor Author

CodeXTL commented Jun 27, 2025

@dellaert Got it! I've removed dim() and restored the wrappers for the Cal3 constructors. I did the following checks afterwards:

  • make check: 100% test pass
  • make python-test: 100% pass

@dellaert dellaert merged commit da36c1f into borglab:develop Jun 28, 2025
39 checks passed
@CodeXTL CodeXTL deleted the fix/removed-dim-from-Cal3 branch June 28, 2025 15:08
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