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

Proposition of API for the method network % evaluate #182

Merged
merged 14 commits into from
Jun 14, 2024

Conversation

jvdp1
Copy link
Collaborator

@jvdp1 jvdp1 commented May 6, 2024

Related to #179

@milancurcic milancurcic marked this pull request as ready for review May 9, 2024 18:07
@milancurcic
Copy link
Member

I added example use of this to examples/dense_mnist.f90.

One challenge I found is that if we use the multiple metrics variant, we can't mix and match functions defined in nf_metrics and those in nf_loss (e.g. metrics=[mse(), corr()] is not allowed because an array constructor must have all types the same).

I think there may be a workaround by first staging metrics as class(metric_type), allocatable :: metrics(:) but I couldn't quite figured it out.

Even if it can't be done, it's a minor limitation IMO.

I'll add some tests too.

@milancurcic
Copy link
Member

@jvdp1 actually, my challenge I mentioned above extends to even passing multiple metrics of the same parent type. Can you show me one example of invoking net % evaluate_batch_1d_metrics()?

@milancurcic
Copy link
Member

I think once we add one example of passing multiple metrics (say, in a test program), we can merge this PR.

@jvdp1
Copy link
Collaborator Author

jvdp1 commented Jun 14, 2024

One challenge I found is that if we use the multiple metrics variant, we can't mix and match functions defined in nf_metrics and those in nf_loss (e.g. metrics=[mse(), corr()] is not allowed because an array constructor must have all types the same).

Indeed, you are right. The same problem happens also with multiple metrics of the same parent type. So, it seems that evaluate_batch_1d_metrics is not possible and should be removed.

@jvdp1
Copy link
Collaborator Author

jvdp1 commented Jun 14, 2024

A possible solution could be this one proposed by Brad. But it sounds to me a bit too complex for what we want to achieve.

@milancurcic
Copy link
Member

Yes, I think this is the same approach that we use to pass an array of heterogeneous layers to a network. It's also overkill for the user to do this. Let's remove the multi-metrics variant then, at least until we hear anyone really needing this.

@milancurcic milancurcic added the enhancement New feature or request label Jun 14, 2024
@milancurcic
Copy link
Member

Thank you!

@milancurcic milancurcic merged commit e82d565 into modern-fortran:main Jun 14, 2024
2 checks passed
@jvdp1 jvdp1 deleted the add_evaluate branch June 14, 2024 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants