Feature Scaling in DoE

[jduerholt]

Hi @Osburg,

this is my interpretation of the scaling as discussed in #354 and #343.

What do you think? No testing etc. yet.

Feel also free, to proceed just from this PR, of course only if you want ;)

Best,

Johannes

[jduerholt]

I also added some tests, and not that I return as jacobian just the diagonal elements of the full matrix.

[Osburg]

I guess you already did most of the work. I mainly integrated your transformations in doe and added a few tests.

[Osburg]

I removed the dependency on n_experiments here. Thought it is not really necessary. Are you fine with this?

[Osburg]

Think here we have to scale x first before calling _evaluate_jacobian() (as it is done now), right?

[Osburg]

Hi @jduerholt,

hmm, on my machine the tests are not failing... possible that they just randomly failed? <-- seems like this is the case.
Apart from this: do my additions roughly look like you wanted it?

Cheers
Aaron :)

[Osburg]

Hi @jduerholt :)

is this one ready to be merged? (don't want to approve my own changes without showing it to sb before :D)

cheers
Aaron

[jduerholt]

Hi @Osburg,

thx for the reminder, I will have a look over the week.

Best,

Johannes

[jduerholt]

Hi @Osburg,

sorry that it took so long. I was in parental leave and could not motivate me.

I have in principle only one comment regarding the design of the classes.

What do you think?

Best,

Johannes

[Osburg]

Hey Johannes,

sry for being inactive for so long, I was very busy with university recently. I removed the TransformEnum and added validators for the feature range. Does it look somewhat similar to what you had in mind?
I think the error in type checking comes from pytorch see here

Cheers
Aaron :)

[jduerholt]

Hi @Osburg,

thank you very much. I pushed a PR in which I created a reusable bound feature: #423 Can you use it also here?

[jduerholt]

In the PR, also the typing issue is fixed.

[jduerholt]

So just merge main into your branch, as the PR is already accepted.

[Osburg]

Hey! in the current version the new Bounds type is used instead. Does it look ok to you now? :)

[jduerholt]

Thank you very much. Looks good to me. If you want you can change the three sugestions that I made. If not, I merge it in as is and change it later.

Just do as you want.

I cannot officially approve it as I am also author of the PR. But I can then force merge it. Just tell me how you want to do it ;)

[Osburg]

Alright, then I approve it. Feel free to merge it in whenever you like :)

[jduerholt]

Perfect, I did not know that this was working. I will then make the two changes this evening and merge it then in. Thanks!