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

Defensive checks for ml labels in config #20

Merged
merged 18 commits into from
Jan 22, 2024
Merged

Conversation

sushidelivery
Copy link
Contributor

This PR implements a verification process to ensure that each classifier label corresponds with the appropriate model and acit files.

@sushidelivery
Copy link
Contributor Author

Hello @9and3 ! Just a little note that this PR needs to be checked, no need to merge it yet! Thank you!

Copy link
Collaborator

@9and3 9and3 left a comment

Choose a reason for hiding this comment

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

Try to check it with some manually added wrong data in the config, e.g.:

  • wrong name labels classifier
  • wrong name model zenodo
  • missing labels classifier

And see if this doesn't break your implementation.

@sushidelivery
Copy link
Contributor Author

Hey @9and3 !

  • wrong name labels classifier and missing labels classifier - fixed in 5f270fe

  • wrong name model zenodo - fixed in 6d123a7

Please, have a look! Thank you !

@sushidelivery
Copy link
Contributor Author

Checked this branch on NUC, seems to be working fine!

@9and3 9and3 changed the title WIP-ADD: adding the label check Defensive checks for ml labels in config Jan 17, 2024
Copy link
Collaborator

@9and3 9and3 left a comment

Choose a reason for hiding this comment

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

We are almost there. I proposed I tiny modification that I think won't alter anything but it's just nicer and tider. Please have a look and let me know when it's done (should not take long).

include/ttool.hh Outdated
Comment on lines 52 to 53
InitializeConfig(ttoolRootPath, configFile);
m_ConfigPtr->CheckAcitFiles(ttoolRootPath);
Copy link
Collaborator

@9and3 9and3 Jan 17, 2024

Choose a reason for hiding this comment

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

As discussed, I think m_ConfigPtr->CheckAcitFiles(ttoolRootPath); should not be exposed in the constructor.
I propose simply to put it in the definition of InitializeConfig(..) function, in the same ttool.hh header. Here:

https://github.com/ibois-epfl/TTool/blob/6e19a12bcba18326c3bfa8cd76d3e66bd2c27488/include/ttool.hh#L433C9-L438C10

 void InitializeConfig(std::string ttoolRootPath, std::string configFile)
        {
            m_ConfigFile = configFile;
            m_ConfigPtr = std::make_shared<ttool::Config>(configFile);
            m_ConfigPtr->SetTToolRootPath(ttoolRootPath);

            # what I propose
            m_ConfigPtr->CheckAcitFiles(ttoolRootPath);
            m_ConfigPtr->CheckClassifierLabelsConfig();

        }

Let me know if it works, and give it a go on a compiled AC just to be 100% sure that there are no problems.

@sushidelivery
Copy link
Contributor Author

Hey @9and3 ! Done it as you proposed, thank you so much! Please, have a look!
P.S. I tested the branch on NUC already.

@9and3 9and3 merged commit 3ae0673 into main Jan 22, 2024
2 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