-
Notifications
You must be signed in to change notification settings - Fork 317
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
Move the solution method 'select case' block to the base class #193
Closed
billsacks
wants to merge
1
commit into
pondedWater/alternativeSolutions_oo
from
pondedWater/alternativeSolutions_oo_v2
Closed
Move the solution method 'select case' block to the base class #193
billsacks
wants to merge
1
commit into
pondedWater/alternativeSolutions_oo
from
pondedWater/alternativeSolutions_oo_v2
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
(Note that the setMetadata method is never called here: a full implementation would need to call that in initialization for each flux that builds on computeFlux_type.) The purpose of this change was to remove what would presumably become duplication of this select case statement throughout the code base. However, I'm not very happy with how this change turned out. The particular challenge here was handling the fact that different fluxes would provide a different set of solution methods. This led me to put stub methods in the base class that die at runtime if they are not overridden, but I feel this makes the use of the base class less obvious and more error-prone. (But I couldn't see a better way to do this - at least, not with introducing a lot of extra complexity.) Whether this extra step is worthwhile depends on how many instances of this select case statement are sprinkled throughout the code, and how much this is cluttering up the code. But overall, given that we potentially have a different set of allowed methods for each flux, I'd prefer not to go with the changes here - instead having some partial duplication of the select case statements. Or we could go with the solution suggested here ea15490#r26526545 of having function pointers, which would allow moving these select case statements to initialization (even though that would still involve many partially-duplicated select case statements throughout the code). Another problem with the changes here is that there will be some performance overhead with the additional subroutine calls - specifically, one extra call to 'solveForFlux' rather than having that inlined. This is likely very bad with the current implementation - point-by-point. With an array implementation, this cost is amortized over all array elements, but may still be non-trivial - and will still be a high cost for single-point runs. -- In some ways, this could be clearer & simpler if we kept computeFlux_type as it was in pondedWater/alternativeSolutions_oo, and instead introduced a separate class to handle the selection between methods. That separate class would have the stub implementations of the various computeFlux methods. Then a flux would be required to provide (some) implementations of the computeFlux* methods, but it would only need to provide an implementation of computeFlux_type (i.e., the getFlux method) if it wanted to call a solution method that required it (like implicitEuler). The instance of the new class would then need to contain an instance of the computeFlux child type, so that it can be referenced by things like comuteFluxImplicitEuler. The main advantage of this would be to provide a more clear signal about whether a given flux can be plugged into things like implicitEuler, which expects an implementation of a particular interface. However, downsides are: - Requires more boilerplate to set up the (now) two classes for a given flux - I think there would be some awkwardness / duplication in terms of the internal variables that are stored in the class: I think these would now be needed in both the computeFlux child class and the new child class. (Though there may be some way to get the computeFlux class to reference things in the new class, e.g., by having a pointer to an object of the new class.) One possible compromise is: keep the computeFlux_type as it was in pondedWater/alternativeSolutions_oo, and make that a base class for the new type. This would provide a more clear signal regarding what's needed in implicitEuler, and a cleaner mechanism to plug a flux into implicitEuler even if it doesn't want to extend the new class (because you could extend computeFlux_type without extending the new class). The new class would provide a default implementation of getFlux, which just calls endrun. Then specific flux implementations would extend the new class, and could then provide implementations for whatever computeFlux* methods they want, as well as possibly providing an implementation for getFlux (if needed). - i.e., the inheritance chain would be: computeFlux_type (abstract interface of getFlux) -> some new class (provides stub implementations for getFlux and various computeFlux* methods) -> specific flux class (provides actual implementations as needed) - But I'm not sure that buys us much relative to what we had in pondedWater/alternativeSolutions_oo: It does make it easier to use implicitEuler (for fluxes that don't want the functionalities of the new class – i.e., the selection between calculation mechanisms), but at the cost of introducing a new class into the design. This could be worth doing if this benefit would be useful in practice. -- Another possibility would be to replace the 'select case' for the solution method with polymorphism. However, this would require having a separate class for each solution method for each flux, which feels like a lot of coding overhead. In this case, stuff shared between the different solution methods – such as data that need to be set initially and the getFlux method – could be in a base class for that flux, and then each solution method would extend that base class, providing a computeFlux method. But I think this is too much overhead. I think a better solution than that would be to ditch the whole select case and solution_method stuff, and go back to my earlier branch (pondedWater/alternativeSolutions_oo as referenced in #192), and go with the idea of using function pointers, as suggested by ea15490#r26526545
Comment from @billsacks Sep 12, 2017 I created this PR to record a solution that I had tried to prototype. But I don't like how it turned out, so I'm closing it - I really just wanted to use the PR mechanism to record this in case I want to come back to it later. |
billsacks
added a commit
that referenced
this pull request
Mar 3, 2023
7b6d92ef6 Merge pull request #198 from johnpaulalex/gitdir 927ce3a98 Merge pull request #197 from johnpaulalex/testpath a04f1148f Merge pull request #196 from johnpaulalex/readmod d9c14bf25 Change the rest of the methods to use -C. Still some usage of getcwd in test_unit_repository_git. 332b10640 Fix incorrect logged path of checkout_externals in test_sys_checkout: it was basically the parent of the current directory, which varies throughout the test. (it called abspath with '{0}/../../', which adds arbitrary and not-interpolated subdir '{0}' to the path, then removes it and removes one more level). 932a7499b Remove printlog from read_gitmodules_file since read_externals_description_file() already has a nearly-the-same printlog (but add it to the other caller). 5d13719ed Merge pull request #195 from johnpaulalex/check_repo 423395449 Update utest to mock _git_remote_verbose in a new way, since it is now called via the GitRepository class rather than on the specific GitRepository instance. d7a42ae96 Check that desired repo was actually checked out. 71596bbc1 Merge pull request #194 from johnpaulalex/manic2 4c96e824e Make the MANIC_TEST_BARE_REPO_ROOT env var special - give it a constant for easy tracking, and automatically tear it down after each test. 259bfc04d test_sys_checkout: use actual paths in on-the-fly configs rather than MANIC_TEST_BARE_REPO_ROOT env var. This will make it easier to test (in the near future) that checkout_externals actually checked out the desired repo dir. 557bbd6eb Merge pull request #193 from johnpaulalex/manic 5314eede1 Remove MANIC_TEST_TMP_REPO_ROOT environment variable in favor of module-level variable. 345fc1e14 Merge pull request #191 from johnpaulalex/test_doc12 2117b843c test_sys_checkout: verify that basic by-tag/branch/hash tests actually take us to the correct git tag/branch/hash. 94d6e5f2b Merge pull request #190 from johnpaulalex/test_doc11 3ff33a6a8 Inline local-path-creation methods 47dea7f64 Merge pull request #189 from johnpaulalex/test_doc10 9ea75cbf8 Grab-bag of renamings: Remove redundant _NAME from repo constants, and consistently add _REPO suffix (This causes the majority of diffs). c0c847ec8 Merge pull request #188 from johnpaulalex/test_doc9 2dd5ce0f7 test_sys_checkout.py: only check for correct 'required' or 'optional' state in the test that exercises required vs optional behavior. Removed a lot of boilerplate. eb3085984 Merge pull request #187 from johnpaulalex/test_doc8 1832e1f84 test_sys_checkout: Simplify many tests to only use a single external. 8689d61ec Merge pull request #186 from johnpaulalex/test_doc7 fbee4253e Grab bag of test_sys_checkout cleanups: Doc inside of each test more clearly/consistently. TestSysCheckoutSVN didn’t get the inlining-of-helper-methods treatment, now it has that. Move various standalone repo helper methods (like create_branch) into a RepoUtils class. README.md was missing newlines when rendered as markdown. Doc the return value of checkout.main Fix test_container_exclude_component - it was looking for the wrong key (which is never present); now it looks for the correct key. f0ed44a6e Merge pull request #185 from johnpaulalex/test_doc6 a3d59f5f2 Merge pull request #184 from johnpaulalex/test_doc5 5329c8ba7 test_sys_checkout: Inline config generation functions that are only called once. 464f2c7a7 test_sys_checkout: Inline another layer (per-config-file checks). Rename the 4 methods that are used multiple times, to reflect what they do rather than what they're called. 8872c0df6 Merge pull request #183 from johnpaulalex/doc_test4 c045335f6 Merge pull request #182 from johnpaulalex/doc_test3 c583b956e Merge pull request #181 from johnpaulalex/doc_test2 e01cfe278 test_sys_checkout: less confusing handling of return values from checkout_externals. Specifically, when doing a checkout, don't return tree_status from _before_ the checkout. Make a new wrapper to call checkout_externals a second time, to calculate the new status after a checkout (very frequent pattern). 23286818c test_sys_checkout: Remove another layer (which generates test component names) c3717b6bc Merge pull request #180 from johnpaulalex/doc_test 36d7a4434 test_sys_checkout.py: remove one layer of functions (that check for local status enums). No-op. 2c4584bf7 More documentation about tests: * contents of test repositories (n a new README.md) * various constants in test_sys_checkout.py that point to those contents, and terminology like container/simple/mixed. * in each test method, the scenarios being tested. * The coupling between test methods. 55e74bd0a Merge pull request #179 from johnpaulalex/circ 66be84290 Remove circular dependency by making _External stop doing tricky things with sourcetrees. git-subtree-dir: manage_externals git-subtree-split: 7b6d92ef689e2f65733e27f8635ab91fb341356b
ekluzek
added a commit
that referenced
this pull request
Dec 16, 2023
0f884bfec Merge pull request #205 from jedwards4b/sunset_svn_git_access 82a5edf79 merge in billsacks:svn_testing_no_github 17532c160 Use a local svn repo for testing 9c904341a different method to determine if in tests 539952ebd remove debug print statement cc5434fa7 fix submodule testing 1d7f28840 remove broken tests 04e94a519 provide a meaningful error message 38bcc0a8c Merge pull request #201 from jedwards4b/partial_match b4466a5aa remove debug print statement c3cf3ec35 fix issue with partial branch match 7b6d92ef6 Merge pull request #198 from johnpaulalex/gitdir 927ce3a98 Merge pull request #197 from johnpaulalex/testpath a04f1148f Merge pull request #196 from johnpaulalex/readmod d9c14bf25 Change the rest of the methods to use -C. Still some usage of getcwd in test_unit_repository_git. 332b10640 Fix incorrect logged path of checkout_externals in test_sys_checkout: it was basically the parent of the current directory, which varies throughout the test. (it called abspath with '{0}/../../', which adds arbitrary and not-interpolated subdir '{0}' to the path, then removes it and removes one more level). 932a7499b Remove printlog from read_gitmodules_file since read_externals_description_file() already has a nearly-the-same printlog (but add it to the other caller). 5d13719ed Merge pull request #195 from johnpaulalex/check_repo 423395449 Update utest to mock _git_remote_verbose in a new way, since it is now called via the GitRepository class rather than on the specific GitRepository instance. d7a42ae96 Check that desired repo was actually checked out. 71596bbc1 Merge pull request #194 from johnpaulalex/manic2 4c96e824e Make the MANIC_TEST_BARE_REPO_ROOT env var special - give it a constant for easy tracking, and automatically tear it down after each test. 259bfc04d test_sys_checkout: use actual paths in on-the-fly configs rather than MANIC_TEST_BARE_REPO_ROOT env var. This will make it easier to test (in the near future) that checkout_externals actually checked out the desired repo dir. 557bbd6eb Merge pull request #193 from johnpaulalex/manic 5314eede1 Remove MANIC_TEST_TMP_REPO_ROOT environment variable in favor of module-level variable. 345fc1e14 Merge pull request #191 from johnpaulalex/test_doc12 2117b843c test_sys_checkout: verify that basic by-tag/branch/hash tests actually take us to the correct git tag/branch/hash. 94d6e5f2b Merge pull request #190 from johnpaulalex/test_doc11 3ff33a6a8 Inline local-path-creation methods 47dea7f64 Merge pull request #189 from johnpaulalex/test_doc10 9ea75cbf8 Grab-bag of renamings: Remove redundant _NAME from repo constants, and consistently add _REPO suffix (This causes the majority of diffs). c0c847ec8 Merge pull request #188 from johnpaulalex/test_doc9 2dd5ce0f7 test_sys_checkout.py: only check for correct 'required' or 'optional' state in the test that exercises required vs optional behavior. Removed a lot of boilerplate. eb3085984 Merge pull request #187 from johnpaulalex/test_doc8 1832e1f84 test_sys_checkout: Simplify many tests to only use a single external. 8689d61ec Merge pull request #186 from johnpaulalex/test_doc7 fbee4253e Grab bag of test_sys_checkout cleanups: Doc inside of each test more clearly/consistently. TestSysCheckoutSVN didn’t get the inlining-of-helper-methods treatment, now it has that. Move various standalone repo helper methods (like create_branch) into a RepoUtils class. README.md was missing newlines when rendered as markdown. Doc the return value of checkout.main Fix test_container_exclude_component - it was looking for the wrong key (which is never present); now it looks for the correct key. f0ed44a6e Merge pull request #185 from johnpaulalex/test_doc6 a3d59f5f2 Merge pull request #184 from johnpaulalex/test_doc5 5329c8ba7 test_sys_checkout: Inline config generation functions that are only called once. 464f2c7a7 test_sys_checkout: Inline another layer (per-config-file checks). Rename the 4 methods that are used multiple times, to reflect what they do rather than what they're called. 8872c0df6 Merge pull request #183 from johnpaulalex/doc_test4 c045335f6 Merge pull request #182 from johnpaulalex/doc_test3 c583b956e Merge pull request #181 from johnpaulalex/doc_test2 e01cfe278 test_sys_checkout: less confusing handling of return values from checkout_externals. Specifically, when doing a checkout, don't return tree_status from _before_ the checkout. Make a new wrapper to call checkout_externals a second time, to calculate the new status after a checkout (very frequent pattern). 23286818c test_sys_checkout: Remove another layer (which generates test component names) c3717b6bc Merge pull request #180 from johnpaulalex/doc_test 36d7a4434 test_sys_checkout.py: remove one layer of functions (that check for local status enums). No-op. 2c4584bf7 More documentation about tests: * contents of test repositories (n a new README.md) * various constants in test_sys_checkout.py that point to those contents, and terminology like container/simple/mixed. * in each test method, the scenarios being tested. * The coupling between test methods. 55e74bd0a Merge pull request #179 from johnpaulalex/circ 66be84290 Remove circular dependency by making _External stop doing tricky things with sourcetrees. 82d3b247f Merge pull request #178 from johnpaulalex/test_doc 3223f49ea Additional documentation of system tests - global variables, method descriptions. 45b7c01c3 Merge pull request #177 from jedwards4b/git_workflow ace90b2c2 try setting credentials this way f4d6aa933 try setting credentials this way 1d61a6944 use this to set git credentials 7f9d330e1 use this to set git credentials 5ac731b85 add tmate code 836847be7 get git workflow working dcd462d71 Merge pull request #176 from jedwards4b/add_github_testing 2d2479e9d Merge pull request #175 from johnpaulalex/fix 711a53fdf add github testing of prs and automatic tagging of main cfe0f888a fix typos 5665d6140 Fix broken checkout behavior introduced by PR #172. 27909e255 Merge pull request #173 from johnpaulalex/readall 00ad0440b Further tiny refactorings and docs of checkout API (no-op). Remove unused load_all param in _External.checkout(). Rename _External.checkout_externals() to checkout_subexternals(), to remove the ambiguity about whether the main external pointed to by the _External is itelf checked out (it is not) Clarify load_all documentation - it’s always recursive, but applies different criteria at each level. Rename variables in checkout.py (e.g. ext_description) to match the equivalent code in sourcetree.py. 2ea3d1a3a Merge pull request #172 from johnpaulalex/fixit 43bf8092c Merge pull request #171 from johnpaulalex/docstatus e6aa7d21e Merge pull request #170 from johnpaulalex/printdir adbd71557 On checkout, refresh locally installed optional packages regardless of whether -o is passed in. add074593 Comment tweaks, and fix 'ppath' typo 696527cb8 Document the format of various status dictionaries, and the various paths and path components within an _External. c677b9403 When processing an external, print out its path in addition to the base filename (to disambiguate all the externals.cfg's) 975d7fd5a Merge pull request #169 from johnpaulalex/docfix_branch 09709e36d Document _Externals.status(). The original comment was apparently copy-pasted from checkout(). 1d880e090 Merge pull request #167 from billsacks/fix_svn_on_windows 3510da848 Tweak a unit test to improve coverage eb7fc1368 Handle the possibility that the URL already ends with '/' 02ea87e3d Fix svn URLs on Windows b1c02ab54 Merge pull request #165 from gold2718/doc_fix 9f4be8c7b Add documentation about externals = None feature a3b3a0373 Merge pull request #162 from ESMCI/fischer/python3 d4f1b1e8d Change shebang lines to python3 2fd941abc Merge pull request #158 from billsacks/modified_solution de08dc2ee Add another option for when an external is in a modified state e954582d0 Merge pull request #156 from billsacks/onbranch_show_hash 952e44d51 Change output: put tag/hash before branch name 10288430f Fix pre-existing pylint issues 01b13f78f When on a branch, show tag/hash, too 39ad53263 Merge pull request #150 from gold2718/fix_combo_config 75f8f02f5 Merge pull request #152 from jedwards4b/sort_by_local_path 42687bd53 remove commented code 29e26af81 fix pylint issues 7c9f3c613 add a test for nested repo checkout 75c5353d2 fix spacing 24a3726a1 improve sorting, checkout externals with each comp 29f45b086 remove py2 test and fix super call 880a4e765 remove decode 1c53be854 no need for set call 36c56dbac simplier fix for issue dc67cc682 simpler solution b32c6fca9 fix to allow submodule name different from path 5b5e1c2b0 Merge pull request #144 from billsacks/improve_errmsg c983863c4 Add another option for dealing with modified externals 59ce252cf Add some details to the error message when externals are modified be5a1a4d7 Merge pull request #143 from jedwards4b/add_exclude 2aa014a1b fix lint issue 49cd5e890 fix lint issues 418173ffd Added tests for ExternalsDescriptionDict afab352c8 fix lint issue be85b7d1b fix the test a580a570b push test d43710864 add a test 21affe33c fix formatting issue 72e6b64ae add an exclude option git-subtree-dir: manage_externals git-subtree-split: 0f884bfec8e43d0c02261de858d6ec3f6d855e51
samsrabin
pushed a commit
to samsrabin/CTSM
that referenced
this pull request
May 3, 2024
fixes an issue in aux data streams
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
NOTE: This PR was originally created in the temporary CTSM repository by @billsacks, Sep 12, 2017, where it was NCAR/clm-ctsm#15. I am moving over the comments from there.
Original comment from @billsacks Sep 12, 2017
(Note that the setMetadata method is never called here: a full
implementation would need to call that in initialization for each
flux that builds on computeFlux_type.)
The purpose of this change was to remove what would presumably become
duplication of this select case statement throughout the code
base. However, I'm not very happy with how this change turned out. The
particular challenge here was handling the fact that different fluxes
would provide a different set of solution methods. This led me to put
stub methods in the base class that die at runtime if they are not
overridden, but I feel this makes the use of the base class less obvious
and more error-prone. (But I couldn't see a better way to do this - at
least, not with introducing a lot of extra complexity.)
Whether this extra step is worthwhile depends on how many instances of
this select case statement are sprinkled throughout the code, and how
much this is cluttering up the code. But overall, given that we
potentially have a different set of allowed methods for each flux, I'd
prefer not to go with the changes here - instead having some partial
duplication of the select case statements. Or we could go with the
solution suggested here
ea15490#r26526545
of having function pointers, which would allow moving these select case
statements to initialization (even though that would still involve many
partially-duplicated select case statements throughout the code).
Another problem with the changes here is that there will be some
performance overhead with the additional subroutine calls -
specifically, one extra call to 'solveForFlux' rather than having that
inlined. This is likely very bad with the current implementation -
point-by-point. With an array implementation, this cost is amortized
over all array elements, but may still be non-trivial - and will still
be a high cost for single-point runs.
--
In some ways, this could be clearer & simpler if we kept
computeFlux_type as it was in pondedWater/alternativeSolutions_oo, and
instead introduced a separate class to handle the selection between
methods. That separate class would have the stub implementations of the
various computeFlux methods. Then a flux would be required to provide
(some) implementations of the computeFlux* methods, but it would only
need to provide an implementation of computeFlux_type (i.e., the getFlux
method) if it wanted to call a solution method that required it (like
implicitEuler). The instance of the new class would then need to contain
an instance of the computeFlux child type, so that it can be referenced
by things like comuteFluxImplicitEuler.
The main advantage of this would be to provide a more clear signal about
whether a given flux can be plugged into things like implicitEuler,
which expects an implementation of a particular interface.
However, downsides are:
flux
internal variables that are stored in the class: I think these would
now be needed in both the computeFlux child class and the new child
class. (Though there may be some way to get the computeFlux class to
reference things in the new class, e.g., by having a pointer to an
object of the new class.)
One possible compromise is: keep the computeFlux_type as it was in
pondedWater/alternativeSolutions_oo, and make that a base class for the
new type. This would provide a more clear signal regarding what's needed
in implicitEuler, and a cleaner mechanism to plug a flux into
implicitEuler even if it doesn't want to extend the new class (because
you could extend computeFlux_type without extending the new class). The
new class would provide a default implementation of getFlux, which just
calls endrun. Then specific flux implementations would extend the new
class, and could then provide implementations for whatever computeFlux*
methods they want, as well as possibly providing an implementation for
getFlux (if needed).
interface of getFlux) -> some new class (provides stub implementations
for getFlux and various computeFlux* methods) -> specific flux class
(provides actual implementations as needed)
pondedWater/alternativeSolutions_oo: It does make it easier to use
implicitEuler (for fluxes that don't want the functionalities of the
new class – i.e., the selection between calculation mechanisms), but
at the cost of introducing a new class into the design. This could be
worth doing if this benefit would be useful in practice.
--
Another possibility would be to replace the 'select case' for the
solution method with polymorphism. However, this would require having a
separate class for each solution method for each flux, which feels like
a lot of coding overhead.
In this case, stuff shared between the different solution methods – such
as data that need to be set initially and the getFlux method – could be
in a base class for that flux, and then each solution method would
extend that base class, providing a computeFlux method.
But I think this is too much overhead. I think a better solution than
that would be to ditch the whole select case and solution_method stuff,
and go back to my earlier branch (pondedWater/alternativeSolutions_oo as
referenced in #192), and go with the
idea of using function pointers, as suggested by
ea15490#r26526545