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

Bugfix for PhaseNet label IDs #277

Merged
merged 7 commits into from Mar 19, 2024
Merged

Conversation

JanisHe
Copy link
Contributor

@JanisHe JanisHe commented Mar 14, 2024

Hi Jannes,

when using the original PhaseNet model or diting as base models for transfer learning, the label IDs are wrong. Default labels_ids are {"P": 0, "S": 1, "Noise": 2}, however, PhaseNet and diting have {"Noise": 0, "P": 1, "S": 2}. Therefore, transfer learning was not possible since the ids did not depend on the given labels by the loaded model. I rewrote the method "_colums_to_dict_and_labels" and added an argument model_labels to get the correct dictionary and label IDs.

Cheers,
Janis

label: [*model_labels].index(label) for label in labels
} # label ids for P and S
if noise_column:
label_ids["Noise"] = len(model_labels) - sum(list(label_ids.values()))
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I understand this formula. I would expect it's just [*model_labels].index("n")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is quite similar to your idea but I wrote it the other way around and add "N" afterwards

@yetinam
Copy link
Member

yetinam commented Mar 14, 2024

Hi @JanisHe ,
thanks a lot for the contribution. Indeed, it's nice to add this flexibility here. I've left a few minor that I'd ask you to address.

I'm not sure I'd call it a bug but it's definitely good to add this functionality. However, that's why I'd prefer merging it into main. Would you mind switching the base branch and rebasing your PR?

@JanisHe
Copy link
Contributor Author

JanisHe commented Mar 14, 2024

Hi Jannes,
I just add your points and then we can merge this to main

@JanisHe JanisHe changed the base branch from maintenance_0.6.x to main March 14, 2024 15:10
@yetinam yetinam added the enhancement New feature or request label Mar 15, 2024
@yetinam yetinam added this to the v0.7 milestone Mar 15, 2024
@yetinam
Copy link
Member

yetinam commented Mar 18, 2024

Hi @JanisHe ,
I'm looking at this now and modifying a few details. I'll hopefully merge it tomorrow.

- Added another test
- Fixed issue with noise label id
- Extended documentation
@yetinam yetinam merged commit 5f3721b into seisbench:main Mar 19, 2024
13 checks passed
@yetinam
Copy link
Member

yetinam commented Mar 19, 2024

I've merged the branch now. Thanks a lot for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants