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

possiblility to pass kwargs to distance function #188

Merged

Conversation

frank-hulo
Copy link
Contributor

In current version it is not possible to pass kwargs if you have a function for the distances. Therefore, added the option to add kwargs.

  • Ability to pass kwargs if a method is used.

@mmaelicke
Copy link
Owner

Hi there,
Thanks for the contribution! Sounds interesting.
I have one little request: As you seem to use an formatting tool, your PR changes quite a lot of code styling. That's fine, but unfortunately I could not easily find the changes you actually made. Could you point them out, so that I can have a look?

Thanks again, best

@frank-hulo frank-hulo marked this pull request as draft August 22, 2024 06:35
Copy link
Owner

@mmaelicke mmaelicke left a comment

Choose a reason for hiding this comment

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

Yeah look good, thanks!

I'll turn this into a PR and run the tests on it.

@mmaelicke mmaelicke marked this pull request as ready for review August 22, 2024 07:00
@frank-hulo
Copy link
Contributor Author

@mmaelicke thank you, it is sufficient. I see metricspace.py is still formatted right. I used black formatter

@mmaelicke
Copy link
Owner

I get failing unittests locally, I try to change the CI to run on PRs of forks, which it does currently not do.
I'll come back to this as soon as I get why the unittests are failing

@mmaelicke
Copy link
Owner

I am not sure, why I can't update this PR to the changes on the main branch. Can you pull the them into your branch manually? Or run the unittests locally? there is a requirements.unittest.3.10.txt for additional test dependencies.

The pytest output is too long to paste it into here and I am not really sure what is going wrong. The main branch is not failling. It is always the same error, as far as I can tell:

self = <skgstat.MetricSpace.MetricSpacePair object at 0x7f79ecd50a40>

    @property
    def dists(self):
        """A distance matrix of all point pairs. If `self.max_dist` is
        not `None` and `self.dist_metric` is set to `euclidean`, a
        `scipy.sparse.csr_matrix` sparse matrix is returned.
        """
        # if not cached, calculate
        if self._dists is None:
            # handle euclidean with max_dist with Tree
            if self.max_dist is not None and self.dist_metric == "euclidean":
                self._dists = self.ms1.tree.sparse_distance_matrix(
                    self.ms2.tree, self.max_dist, output_type="coo_matrix"
                ).tocsr()
    
            # otherwise Tree not possible
            else:
                self._dists = cdist(
                    self.ms1.coords,
                    self.ms2.coords,
>                   metric=self.ms1.dist_metric
                    **self.ms1.dist_metric_kwargs
                )
E               TypeError: unsupported operand type(s) for ** or pow(): 'str' and 'dict'

skgstat/MetricSpace.py:251: TypeError

I guess the self.ms1.dist_metric part got affected by your changes

@frank-hulo
Copy link
Contributor Author

@mmaelicke I forgot to add a comma I see. I've tested locally and it runs fine.

P.S. I do however see some improvements, but should be another PR. The way distance functions are now not really propagated from metricspace to variogram to kriging is a bit confusing IMHO. I would have rather set it only on metricspace and also only allow it as input to variogram and kriging.

@mmaelicke
Copy link
Owner

Works for me too, thanks!

Of course, after testing locally, the Actions picked up the changed CI and the tests ran here as well....

Thanks for your contribution, I will now merge

@mmaelicke mmaelicke merged commit d2aa7f5 into mmaelicke:main Aug 23, 2024
8 checks passed
@mmaelicke
Copy link
Owner

just released as v.1.0.18

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