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

Add first implementation of scaling functions #1197

Draft
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

franflame
Copy link
Contributor

Fixes/Addresses:

#1196

Summary/Motivation:

Adding initial implementation of scaling functions for ML/AI plugin, with tests included.

Legal Acknowledgement

By contributing to this software project, I agree to the following terms and conditions for my contribution:

  1. I agree my contributions are submitted under the copyright and license terms described in the LICENSE.md file at the top level of this directory.
  2. I represent I am authorized to make the contributions and grant the license. If my employer has rights to intellectual property that includes these contributions, I represent that I have received permission to make contributions and grant the required license on behalf of that employer.

@ksbeattie ksbeattie added the Priority:Normal Normal Priority Issue or PR label Mar 5, 2024
@ksbeattie ksbeattie requested a review from bpaul4 March 19, 2024 20:16
return result


# fix expected values in test
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there code missing after this line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing that out, the comment was a note to revise the expected values in test_scaling.py, and the issue has been resolved.

def unscale_log(array_in, lo, hi):
result = lo * np.power(hi / lo, array_in)

# result = ((np.log10(array_in) - np.log10(lo))
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove commented code, if it's not required for anything (same for other commented code in the rest of this file).

return result


class BaseScaler:
Copy link
Contributor

Choose a reason for hiding this comment

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

I may be misunderstanding the usage of the annotations here, but what is the outcome of this class? It seems that arrays that are transformed will raise exceptions for any input.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add a comment to explain the purpose of the BaseScaler class; transform() and inverse_transform() should be implemented by the derived classes, so it raises an error if called from the base class.

@ksbeattie ksbeattie linked an issue May 21, 2024 that may be closed by this pull request
5 tasks
@lbianchi-lbl
Copy link
Contributor

This is affected by the Tensorflow version issues that are being worked on in #1223. This should be considered on hold until those issues have been resolved.

Copy link

codecov bot commented Jul 16, 2024

Codecov Report

Attention: Patch coverage is 90.74074% with 15 lines in your changes missing coverage. Please review.

Project coverage is 38.96%. Comparing base (691185f) to head (3d7b9f8).

Files Patch % Lines
foqus_lib/framework/surrogate/scaling.py 88.80% 11 Missing and 4 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1197      +/-   ##
==========================================
+ Coverage   38.81%   38.96%   +0.14%     
==========================================
  Files         164      165       +1     
  Lines       37048    37178     +130     
  Branches     6153     6159       +6     
==========================================
+ Hits        14382    14486     +104     
- Misses      21528    21553      +25     
- Partials     1138     1139       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority:Normal Normal Priority Issue or PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Streamline scaling functionality
5 participants