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

Proposal to rename imm/3p arguments #98

Closed
venpopov opened this issue Feb 15, 2024 · 1 comment · Fixed by #92
Closed

Proposal to rename imm/3p arguments #98

venpopov opened this issue Feb 15, 2024 · 1 comment · Fixed by #92
Assignees
Labels
PR - minor Pull-request should update minor version
Milestone

Comments

@venpopov
Copy link
Owner

I've been thinking about this for a while, and now is a good time to bring it up - to me spaPos is unintuitive as an argument. Without reading the documentation it sounds like it asks for the position of non_targets, whereas it asks for the distance.

Can I suggest renaming this to spa_dist? More generally, both non_targets and spaPos refer to different features of the non_targets relative to the target. So mayb we should consider something like nt_features/nt_values and nt_dists. That is, I'm suggesting renaming:

non_targets to nt_features or nt_values
spaPos to nt_dists

makes it consistent with the parameter naming (thetant), and makes it clear what type of information each contains and how they are related. Alows opens up the positibility to expand it naturally in the future if we want to allow people to specify multiple distance metrics via nt_dists1, nt_dists2, ntdists3, etc... The model doesn't care what they represent and it would be up to the user to interpret within the context of whatever distance they are using. What do you think?

@GidonFrischkorn
Copy link
Collaborator

Agreed. I have renamed them in #92 commit e9c12dd

non_targets to nt_features, and
spaDist to nt_distance

Generally, I think having the labels a little bist longer and as intuitive as possible will help users a lot.

@GidonFrischkorn GidonFrischkorn added this to the 1.0.0 milestone Feb 16, 2024
@GidonFrischkorn GidonFrischkorn added the PR - minor Pull-request should update minor version label Feb 16, 2024
@GidonFrischkorn GidonFrischkorn self-assigned this Feb 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR - minor Pull-request should update minor version
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants