Skip to content

Refactor: move procedure definitions to submodules #47

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

Closed
wants to merge 2 commits into from

Conversation

rouson
Copy link
Collaborator

@rouson rouson commented Apr 6, 2022

This PR demonstrates an example of separating procedure interface bodies (in a module) from procedure definitions (in a submodule). In current form, the PR applies this process only to what was originally named mod_random.

Benefits:

  1. Reduces the information a user encounters when attempting to use a module.
  2. Clarifies what parts of the module are part of the API. For example,
    • A procedure's local variables no longer appear in the module.
    • A non-public module entity (e.g., pi) no longer appears in the module.

This PR also proposes a new scheme for file, module, and submodule names:

  • Prefix nf_ to reduce name clashes with other projects,
  • Suffix _mod for modules, and
  • Suffix _submod for submodules.

If approved, this PR can be updated to apply similar changes to the rest of the files in src/:

  • mod_activation.f90
  • mod_kinds.f90
  • mod_mnist.f90
  • mod_parallel.f90
  • mod_io.f90
  • mod_layer.f90
  • mod_network.f90

@milancurcic
Copy link
Member

Thanks a lot! Is it possible (allowed) to duplicate the interfaces in the submodule?

Let's keep renames in a separate PR. So this PR would address only submodules, and the other PR would do only renaming. Obviously you need a submodule naming convention in this PR. I think I prefer the more explicit "_submodule" suffix, I hope that's fine with you.

@milancurcic
Copy link
Member

I found out recently that this "naive" implementation of normal distribution is not completely normal and is somewhat skewed toward lower values. It's probably not a big issue (training works as is), but should be fixed nevertheless. Could be a good opportunity to involve Fortran stdlib. Will open a separate issue for that.

@rouson
Copy link
Collaborator Author

rouson commented Apr 6, 2022

@milancurcic thanks for reviewing this so quickly. Yes, it is possible to repeat the interface information in the submodule. I'm in a small minority of programmers who likes not repeating the information so I thought I'd try it this way just see if the change would survive scrutiny. :) The overwhelming majority of people whom I encounter prefer the repetition so your suggestion will reduce the frustration of other contributors. I'll make the changes you requested and then I'll proceed with the rest of the files.

@rouson
Copy link
Collaborator Author

rouson commented Apr 6, 2022

@milancurcic your comments about the normal distribution are timely because I need to write a unit test for a distribution normality for another project. I'll try the tests out on your code.

Also, I pair-programmed this PR with a collaborator who commented that the formulas neural-fortran uses to generate normally distributed data has some problems in the tails. Apparently, the formulas in neural-fortran are fine for most applications so it's not necessarily a cause for concern -- just something to be aware of in case the issue crops up in some way.

@rouson rouson closed this Apr 12, 2022
@rouson rouson deleted the refac-add-submodules branch April 18, 2022 20:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants