Skip to content

Conversation

@alinelena
Copy link
Contributor

this just adds --config to mace_finetunning (which selects samples despite the name).

add --atomic_numbers option

rename it to something more sensible?

@alinelena alinelena marked this pull request as draft April 25, 2025 15:52
@bernstei
Copy link
Collaborator

rename it to something more sensible?

absolutely - what about just prepending mace_ to the name of the function, which is fine_tuning_select? Also, we should make sure that anyplace else in the code we are consistent with finetuning vs. fine_tuning.

@bernstei
Copy link
Collaborator

Do you want to merge this with #965 ?

@alinelena
Copy link
Contributor Author

Do you want to merge this with #965 ?

yes do you want to merge it in? I suspect you can PR it to my fork/branch

@alinelena
Copy link
Contributor Author

Do you want to merge this with #965 ?

@ilyes319 shall we merge that one before?

@alinelena alinelena changed the base branch from main to develop July 29, 2025 22:25
@alinelena alinelena requested a review from bernstei July 29, 2025 23:45
@alinelena alinelena marked this pull request as ready for review July 30, 2025 02:51
@ilyes319
Copy link
Contributor

@alinelena @bernstei Can you both remind me what's the target of that PR, and how that fits in the latest Noam's PRs pipeline?

@bernstei
Copy link
Collaborator

I'm not planning to use this (just filter from inside mace_run_train), although I think it wouldn't be a bad idea for this to exist as a standalone command line interface that does the same data filtering and processing as mace_run_train.

But I think that if there are both, it'd be good to make those command line arguments as consistent as possible.

@alinelena
Copy link
Contributor Author

@bernstei when you filter from mace_run_train, can be quite expensive, especially if you want or have to do the descriptors too. I will rebase and try to get it done

@alinelena alinelena requested a review from bernstei October 29, 2025 20:03
@bernstei
Copy link
Collaborator

Other than the few comments I put in, it looks fine, except there are random spacing changes that lead to more diffs than are actually relevant.

@alinelena alinelena requested a review from bernstei November 4, 2025 07:16
@bernstei
Copy link
Collaborator

bernstei commented Nov 4, 2025

Looks mostly good now, except some random spacing changes in tests/test_run_train_dipole_polar.py and tests/test_run_train.py that aren't meaningful.

Also, is there a test for this CLI script?

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.

3 participants