Sllambias / yucca

Apache License 2.0
17 stars 2 forks source link

Get paths using functions, not global variables and fix local prediction #183

Closed asbjrnmunk closed 2 months ago

asbjrnmunk commented 2 months ago

In general i think that Yucca Functional should be completely agnostic to the underlying folder structure. However, while this will take some more work, this PR changes the handling of path environtment vars such that they are fetched just-in-time, and when actually needed we throw an exception if it is not set instead of a warning.

Sllambias commented 2 months ago

Okay, that's a nice change. I'm not entirely sure how it works though. For example, before you got the warning all the time, presumably because some file imported a path that was not set. Won't you get the same error now because the same files will still be importing paths, right?

Or did I miss a detail that solves this?

However I do see, at least, that maybe now you only get one error for the one path you are trying to import rather than 6 errors. And that's nonetheless no doubt also nice

Sllambias commented 2 months ago

I'm a bit unsure about what feels like using functions as variables in a way

Might just be some brain glitch I have but I think it should look like:

my_variable

And not:

my_variable()

But I think my brain can be eased into this by a PEP saying this is good practice or something like that 😂

asbjrnmunk commented 2 months ago

I'm a bit unsure about what feels like using functions as variables in a way

I'll change it to get_xxx. Leaving out the get is a Ruby convention, and we are not in Ruby, my bad :)

Won't you get the same error now because the same files will still be importing paths, right?

Nope. That is exactly why the old solution is a bit dangerous -- the variables were loaded even if no function would ever use them, because they were loaded in the import phase of the interpreter. So the above works with no more changes and i get no errors.

Sllambias commented 2 months ago

Okay that sounds nice in that case, and yea the "get" will do the trick for my brain I think - all good for me then and nice PR :)

asbjrnmunk commented 2 months ago

This PR now also fixes #184.

asbjrnmunk commented 2 months ago

Just realized it is a bit funny to have a function named get_yucca_preprocessed_data which doesn't return data, but instead returns a path. So i renamed all functions to get_preprocessed_data_path (since the Yucca part is pretty self explanatory since we are inside the Yucca repo :)).

asbjrnmunk commented 2 months ago

@Sllambias will you look at the above changes and re-approve? If you approve it would be lovely if you would merge and update pip! :)) Then i can publish the first AMAES code tomorrow.

asbjrnmunk commented 2 months ago

Context for commit https://github.com/Sllambias/yucca/pull/183/commits/58218857da2711a4831a2bed2d28c131780bdfa1. It means that i can now do the following in AMAES which fixes local prediction:

        logits = self.model.predict(
            data=case_preprocessed,
            ...,
            device=self.device,  <--- new!
        )

in my SupervisedModel lightning module predict_step method, since it knows (in self.device) which device it is on. The implication is that it now again is the L.Trainer.accelerator that determines which device is used during prediction.