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

Aerosol activation NN #318

Merged
merged 1 commit into from
Mar 6, 2024
Merged

Conversation

trontrytel
Copy link
Member

@trontrytel trontrytel commented Feb 9, 2024

This PR brings in the first part of the work of @mikhailmints from last year SURF. It:

  • Creates an extension for the CM function N_activated_per_mode that will be available if the user has the following packages: DataFrames, Flux, MLJ, MLJFlux, CSV, DataFramesMeta, MLJModels
  • Adds a tests that trains a NN model based on PySDM training data and compares the results with the default function.
  • It would be nice to have fewer packages here (maybe we could write it without DataFrames? But lets refactor that once we have all models working first.)

If the jls file with the NN model already exists the training is skipped.

@trontrytel trontrytel force-pushed the ajmm/ml_aerosol_activation_emulation branch from ffdd7fb to 86648c9 Compare February 15, 2024 18:02
@trontrytel trontrytel changed the title Ajmm/ml aerosol activation emulation Aerosol activation emulation Mar 1, 2024
@trontrytel trontrytel force-pushed the ajmm/ml_aerosol_activation_emulation branch from 85621bf to 96e862d Compare March 2, 2024 01:12
@trontrytel trontrytel self-assigned this Mar 2, 2024
@trontrytel trontrytel requested a review from nefrathenrici March 2, 2024 01:29
Copy link
Member

@nefrathenrici nefrathenrici left a comment

Choose a reason for hiding this comment

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

I'm not sure about the MLJ/Flux specifics, but this looks like a good working start.

A stretch goal might be to get rid of DataFrames as a dependency, I am not sure it is necessary

Copy link

codecov bot commented Mar 5, 2024

Codecov Report

Attention: Patch coverage is 85.41667% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 98.11%. Comparing base (13b011d) to head (d38910d).
Report is 1 commits behind head on main.

Files Patch % Lines
ext/Common.jl 82.05% 7 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #318      +/-   ##
==========================================
- Coverage   98.65%   98.11%   -0.55%     
==========================================
  Files          33       35       +2     
  Lines        1117     1165      +48     
==========================================
+ Hits         1102     1143      +41     
- Misses         15       22       +7     

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

@trontrytel
Copy link
Member Author

I have applied all the comments. Thank you @nefrathenrici for all the help with the extensions.

Every now and then I get this warning when training the NN. Do you see where the Float64 is coming in?

[ Info: Training a new ML model. This may take a minute or two.
┌ Warning: Layer with Float32 parameters got Float64 input.
│   The input will be converted, but any earlier layers may be very slow.
│   layer = Dense(11 => 250, sigmoid_fast)  # 3_000 parameters
│   summary(x) = "11×1000 Matrix{Float64}"

@trontrytel trontrytel force-pushed the ajmm/ml_aerosol_activation_emulation branch from a3d4ab7 to 3be25db Compare March 5, 2024 18:45
@trontrytel trontrytel requested a review from nefrathenrici March 5, 2024 18:46
@trontrytel trontrytel marked this pull request as ready for review March 5, 2024 18:47
@trontrytel trontrytel changed the title Aerosol activation emulation Aerosol activation NN Mar 5, 2024
@trontrytel
Copy link
Member Author

Seems like codecov gets confused about the code inside the ext folder. I guess there is no easy way to fix it?

@trontrytel trontrytel added the enhancement New feature or request label Mar 5, 2024
Copy link
Member

@nefrathenrici nefrathenrici left a comment

Choose a reason for hiding this comment

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

This looks good! I think DataFrames really shouldn't be needed here, I can try and remove it when I find the time next week.

I think the Codecov just ignores extensions, I have not been able to find any way around it.

@trontrytel
Copy link
Member Author

This looks good! I think DataFrames really shouldn't be needed here, I can try and remove it when I find the time next week.

Thanks! That would be great, if you have the time.

I think the Codecov just ignores extensions, I have not been able to find any way around it.

So sad. I'm worried my codecov stats will suffer and I will not be able to brag about test coverage anymore. Could we open an issue somewhere on the intercept of codecov and julia extensions communities (assuming it's not empty)...

@trontrytel trontrytel force-pushed the ajmm/ml_aerosol_activation_emulation branch 2 times, most recently from 72b6c1f to ff5d5cf Compare March 6, 2024 00:50
@trontrytel trontrytel force-pushed the ajmm/ml_aerosol_activation_emulation branch from ff5d5cf to d38910d Compare March 6, 2024 16:47
@trontrytel trontrytel enabled auto-merge March 6, 2024 16:55
@trontrytel trontrytel merged commit e2e1a54 into main Mar 6, 2024
8 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants