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

Rename i_df and investigation_df. #529

Merged
merged 4 commits into from
Mar 11, 2024

Conversation

ptth222
Copy link

@ptth222 ptth222 commented Feb 1, 2024

i_df and investigation_df are not a DataFrame as indicated in parameter typing or as the name would suggest. It's actually a dictionary of DataFrames and lists of DataFrames. I have renamed them to reflect this.

Copy link
Member

@proccaserra proccaserra left a comment

Choose a reason for hiding this comment

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

Works but breaks many tests which need upgrading.
On me.

@ptth222
Copy link
Author

ptth222 commented Feb 26, 2024

Interesting. I wouldn't have expected this one to break anything since it just renames variables/parameters. I am looking into the testing now to try and understand how it all works to try and make changes to them so some of these PRs will pass.

@ptth222
Copy link
Author

ptth222 commented Mar 1, 2024

That's strange. I don't see @terazus comments, but I got an email about it. I think you are right. I will look through the tests and try to find the renaming changes.

ptth222 added 3 commits March 7, 2024 12:57
The last changes were initially tested and created on Windows, but had errors on GitHub. I fixed those errors, but didn't retest on Windows. This has been tested on both and is problem free.
i_df and investigation_df are not a DataFrame as indicated in parameter typing or as the name would suggest. It's actually a dictionary of DataFrames and lists of DataFrames. I have renamed them to reflect this.
@ptth222 ptth222 force-pushed the tab-validate-idf-rename branch from 3a9a96c to 299a93d Compare March 7, 2024 18:36
@ptth222 ptth222 changed the base branch from develop to issue-511 March 7, 2024 18:47
@ptth222
Copy link
Author

ptth222 commented Mar 7, 2024

I did not mean to trigger a rerun on this when I rebased it. I am working on looking at the broken tests now.

There were 2 tests that needed to be changed to match the new naming.
@coveralls
Copy link

Coverage Status

coverage: 81.298% (-0.001%) from 81.299%
when pulling 33dae30 on ptth222:tab-validate-idf-rename
into e5f6fd2 on ISA-tools:issue-511.

@terazus
Copy link
Collaborator

terazus commented Mar 8, 2024

I did not mean to trigger a rerun on this when I rebased it. I am working on looking at the broken tests now.

You can mark PR as draft if you are not ready yet.

@ptth222
Copy link
Author

ptth222 commented Mar 8, 2024

@terazus It is ready now. It initially ran when I did the rebase because the PR was already here and I think it was previously approved. The last commit I made triggered another rerun, but that is good to go.

@terazus terazus merged commit 01dffc7 into ISA-tools:issue-511 Mar 11, 2024
5 checks passed
@ptth222 ptth222 deleted the tab-validate-idf-rename branch March 20, 2024 22:27
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.

4 participants