Re: [MATH][GENETICS] Review of PR #197
Le ven. 29 oct. 2021 à 17:00, Avijit Basak a écrit : > > Hi All > > I have fixed most of the review comments. The changes have been > committed to PR#199. > > (A) > Please "rebase" on "master". > Please "squash" intermediate commits: For a new feature, a single commit > should exist (that corresponds to the JIRA report describing it). > --Will be done once all changes are finalized and committed. What is the rationale for not doing it right now? The PR should always be "rebased" on the latest "master". > > (B) > I'm confused by your defining "legacy" packages in new modules... > What kind of comparisons are you considering? > It is fine to depend (with scope "test") on CM v3.6.1 to perhaps (?) > regression testing; but please note that when your proposal is > merged, it will imply that the "legacy" codes *must* be removed. > [We don't want to keep duplicate functionality.] > -- The new implementation has improved the quality of optimization over the > legacy model. "Improved" in what sense? If you mean enhanced performance, such checks should be done using JMH (producing data to be published on the web site). > I have added the legacy packages to demonstrate the same. > Once we remove the genetics packages in the legacy module, the same will be > deleted from examples. I'm probably missing what exactly those "legacy" examples aim to demonstrate... In passing, what's the purpose of Thread.sleep(5000) (at line 55 in file "TSPOptimizerLegacy")? > > (C) > File > "commons-math-examples/examples-ga/src/main/resources/spotbugs/spotbugs-exclude-filter.xml" > does not belong there. > --Removed > > (D) General design > Class "ConsoleLogger" > -> We should not reinvent the wheel. We should consider whether logging > is necessary, and in the affirmative, depend on the de facto standard: > "slf4j". > -- Introduced the slf4j with simple implementation and removed the > consolelogger class. Could you please post a separate message highlighting the new dependency? Also (if no objections are made): 1. the declaration should go in the main POM (in the section). 2. the library (CM) must not depend on a specific logger implementation. > > Class "Constants" > -> Any data should be declared where its purpose is obvious. > --Put the constants in relevant classes. > > * Class "RandomGenerator" > 1. Duplicates functionality (storage of thread-local instances) > readily available in "Commons RNG". > 2. (IMHO) Thread-local instances should not be used for "heavy" usage > (like in GA). > --Modified You still use "ThreadLocalRandomSource". I think that if some functionality requires a RNG instance it should be passed to its constructor (or created by it). I noticed that the interface "ChromosomeRepresentationUtils" is actually a utility class (as the name indicates): It only contains "static" functions. This will probably require refactoring (utility classes are frowned upon nowadays). Some functionality in that class (e.g. sampling from a range of integers) exists in the "sampling" module of "Commons RNG". > > * Class "ValidationUtils" > -> should not be public (or should be defined in an "internal" package). > --Changed The class actually provides no added value. > > > > * Class "AbstractListChromosome" (and subclasses) > > Didn't we conclude that this was a very wasteful implementation of the > > "chromosome" concept? > > -- I have some concerns regarding this. I am not much aware of any > > discussion regarding this conclusion. > > >Please search the ML archive; I seem to recall a detailed discussion > >where Alex gave hints on how a binary chromosome should be > >implemented. > --There is a separate Jira for the same. I shall do the implementation. > > > Chromosomes are always conceptualized as collections of allele/genes. So > We > > need a collection of the *genotypes* anyway. Here List has been used as a > > collection. > > We need an abstraction for representing the collection of Genotype. All > > crossover and mutation operators are based on this abstraction. This > > enabled reuse of crossover and mutation operators for all chromosome types > > which extend the abstraction. I am not sure how to achieve this > reusability > > without an abstraction. The abstraction is quite useful, but the point is indeed that the abstraction is somewhat broken by exposing the List as the representation. I understand that the advantage is to be able to define several functionalities (crossover, ...) in a generic way; but is it worth the obvious drawback is that object instances must be used to represent genes? > > Any domain specific new chromosome implementation extending the > > AbstractListChromosome class can reuse all crossover and mutation > operators. > > For our proposed improvement of BinaryChromosome we should be able to > > extend the AbstractChromosome (*not* AbstractListChromosome) for the new > > class and provide the dedicated crossover and mutation operators for the > > corresponding
Re: [All] What belongs to "changes.xml"
Feel free to edit as you see fit ;-) Gary On Fri, Oct 29, 2021, 20:26 sebb wrote: > I'm inclined to agree with Gilles that updates to plugins should only > be noted where they affect users of the code, i.e. direct > dependencies. > > The useful changes are being shouted down by the various plugin changes. > > In particular, updates to actions are completely irrelevant as they > only affect our GitHub installation. > > On Sat, 30 Oct 2021 at 01:13, Gary Gregory wrote: > > > > IMO, each component can do as it best sees fit. I like the idea of the > file > > being a summary of all changes. RE updates to dependencies and plugins, > > these will matter to some but not others, it depends on how you use the > > component. I see it as making visible and transparent the kind of > attention > > to detail we have (or not). > > > > Gary > > > > On Fri, Oct 29, 2021, 18:36 Gilles Sadowski > wrote: > > > > > Hello. > > > > > > Referring to e.g. the recent > > >https://commons.apache.org/proper/commons-cli/changes-report.html > > > I notice that almost half the reported changes concern build's plugins > > > updates. > > > Shouldn't "changes.xml" only report modifications of the code source, > i.e. > > > things that are of direct interest to users of the component? > > > > > > Regards, > > > Gilles > > > > > > - > > > To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org > > > For additional commands, e-mail: dev-h...@commons.apache.org > > > > > > > > - > To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org > For additional commands, e-mail: dev-h...@commons.apache.org > >
Re: [All] What belongs to "changes.xml"
I'm inclined to agree with Gilles that updates to plugins should only be noted where they affect users of the code, i.e. direct dependencies. The useful changes are being shouted down by the various plugin changes. In particular, updates to actions are completely irrelevant as they only affect our GitHub installation. On Sat, 30 Oct 2021 at 01:13, Gary Gregory wrote: > > IMO, each component can do as it best sees fit. I like the idea of the file > being a summary of all changes. RE updates to dependencies and plugins, > these will matter to some but not others, it depends on how you use the > component. I see it as making visible and transparent the kind of attention > to detail we have (or not). > > Gary > > On Fri, Oct 29, 2021, 18:36 Gilles Sadowski wrote: > > > Hello. > > > > Referring to e.g. the recent > >https://commons.apache.org/proper/commons-cli/changes-report.html > > I notice that almost half the reported changes concern build's plugins > > updates. > > Shouldn't "changes.xml" only report modifications of the code source, i.e. > > things that are of direct interest to users of the component? > > > > Regards, > > Gilles > > > > - > > To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org > > For additional commands, e-mail: dev-h...@commons.apache.org > > > > - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
Re: [All] What belongs to "changes.xml"
IMO, each component can do as it best sees fit. I like the idea of the file being a summary of all changes. RE updates to dependencies and plugins, these will matter to some but not others, it depends on how you use the component. I see it as making visible and transparent the kind of attention to detail we have (or not). Gary On Fri, Oct 29, 2021, 18:36 Gilles Sadowski wrote: > Hello. > > Referring to e.g. the recent >https://commons.apache.org/proper/commons-cli/changes-report.html > I notice that almost half the reported changes concern build's plugins > updates. > Shouldn't "changes.xml" only report modifications of the code source, i.e. > things that are of direct interest to users of the component? > > Regards, > Gilles > > - > To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org > For additional commands, e-mail: dev-h...@commons.apache.org > >
[All] What belongs to "changes.xml"
Hello. Referring to e.g. the recent https://commons.apache.org/proper/commons-cli/changes-report.html I notice that almost half the reported changes concern build's plugins updates. Shouldn't "changes.xml" only report modifications of the code source, i.e. things that are of direct interest to users of the component? Regards, Gilles - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
[ANNOUNCE] Apache Commons CLI 1.5.0
The Apache Commons project announces the release of Apache Commons CLI 1.5.0. Commons CLI requires at least Java 8. The Apache Commons CLI library provides a simple command Line arguments parser. This is a Feature and bug fix release (Java 8) Historical list of changes: https://commons.apache.org/proper/commons-cli/changes-report.html For complete information on Apache Commons CLI, including instructions on how to submit bug reports, patches, or suggestions for improvement, see the Apache Apache Commons CLI website: https://commons.apache.org/proper/commons-cli/ Download page: https://commons.apache.org/proper/commons-cli/download_cli.cgi Have fun! - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
Re: [MATH][GENETICS] Review of PR #197
Hi All I have fixed most of the review comments. The changes have been committed to PR#199. (A) Please "rebase" on "master". Please "squash" intermediate commits: For a new feature, a single commit should exist (that corresponds to the JIRA report describing it). --Will be done once all changes are finalized and committed. (B) I'm confused by your defining "legacy" packages in new modules... What kind of comparisons are you considering? It is fine to depend (with scope "test") on CM v3.6.1 to perhaps (?) regression testing; but please note that when your proposal is merged, it will imply that the "legacy" codes *must* be removed. [We don't want to keep duplicate functionality.] -- The new implementation has improved the quality of optimization over the legacy model. I have added the legacy packages to demonstrate the same. Once we remove the genetics packages in the legacy module, the same will be deleted from examples. (C) File "commons-math-examples/examples-ga/src/main/resources/spotbugs/spotbugs-exclude-filter.xml" does not belong there. --Removed (D) General design Class "ConsoleLogger" -> We should not reinvent the wheel. We should consider whether logging is necessary, and in the affirmative, depend on the de facto standard: "slf4j". -- Introduced the slf4j with simple implementation and removed the consolelogger class. Class "Constants" -> Any data should be declared where its purpose is obvious. --Put the constants in relevant classes. * Class "RandomGenerator" 1. Duplicates functionality (storage of thread-local instances) readily available in "Commons RNG". 2. (IMHO) Thread-local instances should not be used for "heavy" usage (like in GA). --Modified * Class "ValidationUtils" -> should not be public (or should be defined in an "internal" package). --Changed > > * Class "AbstractListChromosome" (and subclasses) > Didn't we conclude that this was a very wasteful implementation of the > "chromosome" concept? > -- I have some concerns regarding this. I am not much aware of any > discussion regarding this conclusion. >Please search the ML archive; I seem to recall a detailed discussion >where Alex gave hints on how a binary chromosome should be >implemented. --There is a separate Jira for the same. I shall do the implementation. > Chromosomes are always conceptualized as collections of allele/genes. So We > need a collection of the *genotypes* anyway. Here List has been used as a > collection. > We need an abstraction for representing the collection of Genotype. All > crossover and mutation operators are based on this abstraction. This > enabled reuse of crossover and mutation operators for all chromosome types > which extend the abstraction. I am not sure how to achieve this reusability > without an abstraction. > Any domain specific new chromosome implementation extending the > AbstractListChromosome class can reuse all crossover and mutation operators. > For our proposed improvement of BinaryChromosome we should be able to > extend the AbstractChromosome (*not* AbstractListChromosome) for the new > class and provide the dedicated crossover and mutation operators for the > corresponding Genotype. Without an *explicit* abstraction, management of > crossover and mutation operators would be difficult. > Please share further thoughts regarding this. Whether we should have a data-based (subclass of "List") or behaviour-based access to individual genes is a fundamental design decision. The "legacy" implementation chose the former but it might be worth considering an alternative (lest we'll need to support several APIs if we later come to the conclusion that we need a more compact representation for some use cases. --How are you thinking the design to be behaviour-based. It will be helpful if you share some examples. The legacy was only based on data types. But the current model introduces the concept of phenotype along with genotype. The genotype (List) represents the actual chromosome implementation and the phenotype () represents the problem domain. A Decoder is also introduced to convert genotype to phenotype. In this way this design would be capable of using a single genotype for a wide variety of problem domains(phenotype) including both direct and indirect encoding. Do you see any challenge to this current implementation? Kindly share your thoughts. * Package "convergencecond" -> Should probably be renamed "convergence". --Done * Class "GeneticException" 1. Should not be public (or should be defined in an "internal" package"[1]). 2. If various types (that map to different JDK subclasses of RuntimeException, e.g. "IllegalArgumentException" vs "NullPointerException") are necessary, there could be a factory for creating the appropriate instance. However, for "null" checks, please use the JDK utilities[2]. --Moved to an internal package. Null checks have been modified too. > > * Class "ConvergenceListenerRegistry" > Shouldn't it be thread-safe? > -- Yes. We need this to be
[ANNOUNCE] Apache Commons CLI 1.5.0
The Apache Commons project announces the release of Apache Commons CLI 1.5.0. Commons CLI requires at least Java 8. The Apache Commons CLI library provides a simple command Line arguments parser. This is a Feature and bug fix release (Java 8) Historical list of changes: https://commons.apache.org/proper/commons-cli/changes-report.html For complete information on Apache Commons CLI, including instructions on how to submit bug reports, patches, or suggestions for improvement, see the Apache Apache Commons CLI website: https://commons.apache.org/proper/commons-cli/ Download page: https://commons.apache.org/proper/commons-cli/download_csv.cgi Have fun! - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
