Feature/composite types by thelmuth · Pull Request #28 · erp12/cbgp-lite

thelmuth · 2023-07-02T18:42:06Z

Mostly adds composite types problems to CBGP. But also makes a bunch of smaller changes.

…te-types

Refactors composite suite case generators

Fixes `penalize-nil` in composite suite for all collections

…te-types

…d up some things

…nge.

…te-types

…f a composite type were found.

… take two of the same type as arguments.

…search/plushy.clj

thelmuth · 2023-07-05T21:46:18Z

I just renamed src/erp12/cbgp_lite/search/pluhsy.clj to src/erp12/cbgp_lite/search/plushy.clj, since it has always confused me.

erp12 · 2023-07-09T19:21:51Z

I just renamed src/erp12/cbgp_lite/search/pluhsy.clj to src/erp12/cbgp_lite/search/plushy.clj, since it has always confused me.

LOL. My editor's autocomplete saved me from ever noticing that typo. Thank!

erp12

Overall looks good to me. Just a few minor questions and suggestions. All of these comments also apply to #29.

I am going to have to think about how best to merge this PR and #29 given the large number of shared commits. Some of these commits seem to have incomplete code, and should be squashed into a smaller set of larger commits that form a working version at each commit. If there is no clean separation of commits it might be best to squash everything into 1 commit. This will generate new commit hashes for each PR and there will be a lot of conflicts.

I think the best thing to do is redirect these PRs into a new branch in this repository and then I can handle the rebasing and squashing from there before merging to master.

In the future, it would be good to make a few workflow changes to keep the PRs easy to work with:

Pull this repo's main branch to your fork's main branch before creating the feature branch.
Using rebase to pull commits from other branches rather than merge. Here is a nice article about the why and how and there is a lot more discussion about this online.
If 2 feature branches depend on some of the same changes, submit an initial PR with those changes and get that merged starting both feature branches. It's always more difficult to synchronize changes to multiple moving targets.

erp12 · 2023-07-09T19:26:57Z

The local_runner.py script has configuration for launching single runs (example). What is the motivation for having a separate script?

I made this script before local_runner.py supported single runs, and haven't had time to change my tools to use local_runner.py.

Big picture, last week I made a version of my cluster launching code that doesn't use either of these python scripts and just calls Clojure directly. I made this change because it was a pain to add new command line args to the python scripts, and I didn't see a reason to have another layer of code. So, this file can probably be deleted.

Sounds good.

erp12 · 2023-07-09T19:34:49Z

I don't love having this much site specific information in a public repository, but we have historically done a similar thing for fly. I'm fine with including this for now because we can expect most users to be our collaborators, but we should add a big warning in some hard to miss place that this is for the maintainers of this project only.

Also, please move this to the scripts folder. Perhaps a README.md in that folder would be a good place for details who each script is for.

It would be fine to delete these and have me keep them in a branch in my fork, if that's easier.

I'm sure there will be future collaborators who will be developing against the HPC who will want to use these files. As long as you are okay with that code and documentation being public, I think it can be merged so that we don't have to maintain multiple long-lived branches indefinitely.

Moving the collaborator-only files to scripts/ should help reduce confusion to the wider audience.

erp12 · 2023-07-09T19:35:06Z

This should also be moved to the scripts folder.

erp12 · 2023-07-09T19:39:04Z

+  (read-cases {:data-dir "data/psb/"
+               :problem "substitution-cipher"
+               :n-train 10
+               :n-test 0})
+  ;; => {:train
+  ;;     ({:inputs ["" "" ""], :output ""}
+  ;;      {:inputs ["a" "a" "a"], :output "a"}
+  ;;      {:inputs ["j" "h" "j"], :output "h"}
+  ;;      {:inputs ["a" "z" "a"], :output "z"}
+  ;;      {:inputs ["e" "l" "eeeeeeeeee"], :output "llllllllll"}
+  ;;      {:inputs ["h" "d" "hhhhhhhhhhhhhhhhhhhh"], :output "dddddddddddddddddddd"}
+  ;;      {:inputs ["o" "z" "oooooooooooooooooooooooooo"], :output "zzzzzzzzzzzzzzzzzzzzzzzzzz"}
+  ;;      {:inputs ["abcdefghijklmnopqrstuvwxyz" "zyxwvutsrqponmlkjihgfedcba" "bvafvuqgjkkbeccipwdfqttgzl"],
+  ;;       :output "yezuefjtqppyvxxrkdwujggtao"}
+  ;;      {:inputs ["abcdefghijklmnopqrstuvwxyz" "cdqutzayxshgfenjowrkvmpbil" "thequickbrownfxjmpsvlazydg"],
+  ;;       :output "kytovxqhdwnpezbsfjrmgcliua"}
+  ;;      {:inputs ["otghvwmkclidzryxsfqeapnjbu" "alpebhxmnrcyiosvtgzjwuqdfk" "aaabbbccc"], :output "wwwfffnnn"}),
+  ;;     :test ()}
+
+  (read-cases {:data-dir "data/psb/"
+               :problem "gcd"
+               :n-train 10
+               :n-test 0})
+  ;; => {:train
+  ;;     ({:inputs [1 1], :output 1}
+  ;;      {:inputs [4 400000], :output 4}
+  ;;      {:inputs [54 24], :output 6}
+  ;;      {:inputs [4200 3528], :output 168}
+  ;;      {:inputs [820000 63550], :output 2050}
+  ;;      {:inputs [123456 654321], :output 3}
+  ;;      {:inputs [524221 135232], :output 1}
+  ;;      {:inputs [586650 803185], :output 5}
+  ;;      {:inputs [347099 142029], :output 1}
+  ;;      {:inputs [902215 966305], :output 5}),
+  ;;     :test ()}
+
+


Is this debug code that got left behind or is it trying to document something for the reader?

Debug code.

erp12 · 2023-07-09T19:46:14Z

+      {:description    "Given a map from strings to ints and two strings that are
+                     keys of the map, look up the values associated with those keys
+                     in the map and return their sum."


Minor formatting wonky-ness in these description strings and they contains all those newlines and spaces. Consider something like:

:description (str "Given a map from strings to ints and two strings that are " "keys of the map, look up the values associated with those keys " "in the map and return their sum.")

Good idea, but I don't have time to make all of these changes.

That's fine. Let's just add a @todo somewhere in this file to fix description formatting.

erp12 · 2023-07-09T19:54:53Z

+  ;; Current number of problems
+  (count (keys (problems {:penalty nil})))
+
+  (map :loss-fns (vals (problems {:penalty nil})))


What is this line trying to show?

Nothing, just development code that didn't get removed.

thelmuth · 2023-07-09T21:45:41Z

In the future, it would be good to make a few workflow changes to keep the PRs easy to work with:

1. Pull this repo's main branch to your fork's main branch before creating the feature branch.

2. Using `rebase` to pull commits from other branches rather than `merge`. [Here is a nice article about the why and how](https://simondosda.github.io/posts/2022-01-03-git-rebase-workflow.html) and there is a lot more discussion about this online.

3. If 2 feature branches depend on some of the same changes, submit an initial PR with those changes and get that merged starting both feature branches. It's always more difficult to synchronize changes to multiple moving targets.

This all seems like a good idea if we want this to be a codebase that's really well organized and documented. However, it seems like it could be a huge pain if you're working on multiple features at once that branch off of each other. If you rebase all of these commits, then the branches my students have been working on that are based on these commits will be a pain to clean up, right? Or is it easier than I'm imagining (I've never used a rebase workflow)?

In all honesty, I would find it much more helpful if efforts to make this a nice codebase were used to better document the code, rather than worrying about keeping a clean git history. But maybe we can have both?

erp12 · 2023-07-13T02:23:43Z

This all seems like a good idea if we want this to be a codebase that's really well organized and documented. However, it seems like it could be a huge pain if you're working on multiple features at once that branch off of each other.

IMHO, this is a normal part of collaborating on long-term software projects. I agree that the scientific nature of our work raises additional wrinkles around supporting reproducibility and ephemeral experimentation code, but I think those are fairly well covered by a typical usage of version control.

It's fine to make a branch for experiments with no intention of merging that new version upstream. That version should be adequate for reproducibility. If we learn something from those experiments and would like to change the "main" version from which all other people will branch off for the whole life of the codebase, we can find the best way to make that change. It doesn't necessarily have to be merging the exact experimental version (but it might be). If the experiment does not lead us to a definitive change we want to adopt more broadly, it can stay in it's branch for posterity but doesn't need to complicate the main version.

In short, I would like this repository to be well organized and documented to the sake of longevity. I think we all have been through a few cycles Clojure PushGP rewrites that all take the philosophy of being an accretive bag of past experiments. I don't love that model, especial after getting burned by my mistakes in pyshgp.

If you rebase all of these commits, then the branches my students have been working on that are based on these commits will be a pain to clean up, right?

This will indeed be painful. This is why, if you have dependency between changes, it is often encouraged to do multiple smaller branches in series as opposed to multiple levels of branching. In cases where there are multiple levels of branches, the usual approach to have every branch merge into the branch they branched from. In this case, assuming your students all branched off of your (this?) branch that would mean they should all first merge their changes into your branch and then afterwards your branch would be merged.

But! I think I have a workflow for our current situation that will let me take over most of the painful merging. I have created a branch off of master called premerge_28_20 and pointed both PRs to be merging into that branch. Once we finish going through the review comments on both PRs we can merge them with no rebasing into the premerge branch. There may be some conflicts to resolve but hopefully it won't be too bad. I will take over from there and rebase the premerge branch and open a new PR against master.

In all honesty, I would find it much more helpful if efforts to make this a nice codebase were used to better document the code, rather than worrying about keeping a clean git history. But maybe we can have both?

Both great qualities to strive for, but orthogonal no? :)

FWIW, shorter lived PRs and a git history on master of only working version with a single logical change per commit is valuable beyond using git commit messages as a form of documentation. In fact, I find the documentation usage of commits the least valuable part. I'll give 3 hypothetical scenarios that happen a non-trivial amount of time:

This branch has been out for 6 months and could hypothetically have included a ton of changes across all areas of the codebase (it doesn't, but let's imagine). Suppose I had opened another branch more recently, but we merged this branch before I merged mine. Much of my work could need to be redone to account for the many changes.
Suppose we find evidence of a bug and want to identify which change introduced the bug. Ideally we would be able to search through the history of master and see other working versions of the code (net of undiscovered/unfixed bugs) and not have to piece together the real changes from partially written dev code spread out over multiple of commits. We can only get use of git blame if the commits are meaningful.
Suppose we have code that used to exist in the main version but no longer does. If we want to find that code we should be able to search through the commits on master and only come up with complete (and if well tested, hopefully correct) versions of the code. It's painful to play detective across a dozen commits with scratch code and half baked implementations.

In short, caring about the git history is much more about the maintainers than the users.

thelmuth and others added 30 commits January 14, 2023 21:13

Adds penalties for nil program outputs.

c157d58

Fix small bug in single_run_runner.py

7e5d8ab

Merge remote-tracking branch 'epantridge/master'

c02d059

Merge remote-tracking branch 'epantridge/master'

ff35d18

Merge remote-tracking branch 'epantridge/master'

739e12a

Merge useful files for Hamilton's HPC

a8c3fb0

Added clojure command line args to single_run_runner

812fe5a

Fixed single runner that I broke

02e327b

First problem using map as input

0e83aa0

Added a problem description

722be6a

Implemented the two PSB2 problems that require 2 outputs, as a 2-tuple

f7e2875

Added polymorphic problem

f643bd0

Got add-them problem working

d90c488

Added sum-2D problem

f1f9cd2

Fixed 2D inputs to vectors instead of lists

f4d59dd

Added centimeters-to-meters problem

c71d8b6

Added set-symmetric-difference problem and bu/jaccard-similarity-loss

4947c32

Updated polymorphic type:

5c53d7a

Merge remote-tracking branch 'epantridge/master' into feature/composi…

fec09dd

…te-types

Fixed issue with input to sum-2D problem

da829ce

Added max-applied-fn problem

e0813b7

Added count-true problem

9b6a927

Added first-index-of-true problem

02e98bd

Added set-cartesian-product and filter-bounds problems. Fixed <

daf7c0f

Added area-of-rectangle problem. Also some minor debugging

b791da8

Refactors composite suite case generators

b43fad3

Merge pull request #1 from erp12/erp_composite_types_fix

9937fdc

Refactors composite suite case generators

Reverted change that broke ga.clj

b2b21b8

Fixes penalize-nil in composite suite for all collections

540e439

Merge pull request #2 from erp12/erp_composite_penalize_nil

99100af

Fixes `penalize-nil` in composite suite for all collections

thelmuth added 18 commits February 8, 2023 16:40

Merge remote-tracking branch 'epantridge/master' into feature/composi…

a0535a9

…te-types

Added sum-vector-vals function

2384bb3

Added index-based loss function for filter-bounds:

9422205

Added problem sets-with-element

641d96e

Added timesheet problem

eb4fc60

Merge remote-tracking branch 'epantridge/master' into feature/composi…

65c2812

…te-types

Fixed PSB2 2-tuple problems. Removed add-them testing problem. Cleane…

4a2bff8

…d up some things

Added min-key problem. Cleaned up a few things without functional cha…

cc1dd54

…nge.

Added simple-encryption problem

6c253ee

Merge remote-tracking branch 'epantridge/master' into feature/composi…

6d8d6b3

…te-types

Added get-vals-of-key problem

5c19e3e

Fixed bug in first-index-of-true problem

4f02347

Fixed bug in max-applied-fn case generator

af52a1f

Renamed timesheet to time-sheet

e0a7b19

Cleaned up types for Composite problems. No issues of assumed child o…

e99e195

…f a composite type were found.

Cleaned up types in PSB. Didn't find any poorly typed problems.

ab53f80

Cleaned up composite.clj. Removed duplicate safe-nth. Made = and not=…

b212721

… take two of the same type as arguments.

Renamed src/erp12/cbgp_lite/search/pluhsy.clj to src/erp12/cbgp_lite/…

1765de7

…search/plushy.clj

erp12 requested changes Jul 9, 2023

View reviewed changes

erp12 changed the base branch from master to premerge_28_29 July 9, 2023 20:13

erp12 mentioned this pull request Jul 9, 2023

Summer2023/add new psb2 instructions #29

Open

erp12 mentioned this pull request Oct 22, 2023

Composite types suite & functions from PSB2 paper #33

Merged

Conversation

thelmuth commented Jul 2, 2023

Uh oh!

thelmuth commented Jul 5, 2023

Uh oh!

erp12 commented Jul 9, 2023

Uh oh!

erp12 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thelmuth commented Jul 9, 2023

Uh oh!

erp12 commented Jul 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants