Skip to content

Jh refactor 6 - #18

Merged
wkearn merged 3 commits into
TopoToolbox:mainfrom
jkhillier:JH_refactor_6
Jul 28, 2026
Merged

Jh refactor 6#18
wkearn merged 3 commits into
TopoToolbox:mainfrom
jkhillier:JH_refactor_6

Conversation

@jkhillier

Copy link
Copy Markdown
Contributor

Following your guidance. Seems to still be working. FYI: Not entirely why - in detail - I'm refactoring, but happy to keep going.

@jkhillier

Copy link
Copy Markdown
Contributor Author

Hi William - Please can you have a look at why the checks are failing. "make check" works on my machine. I've stopped a couple of the temp files outputting. Could this be it? Thank you. John

@jkhillier

Copy link
Copy Markdown
Contributor Author

Ah - I left two variables in on L1454. What's the cleanest way to proceed? (i.e. GitHub process). I'll wait for you to tell me so that I don't mess things up ..... I don't want to create another pull-request for you if it messes up workflow.

@wkearn

wkearn commented Jul 27, 2026

Copy link
Copy Markdown
Member

Make sure you are still on your JH_refactor_6 branch and remove those variable declarations. Then use git add src/swt.c and git commit -m "Remove leftover variable declarations" to make a new commit and git push origin JH_refactor_6 to push your changes to your branch on GitHub. GitHub recognizes when new commits have been added to a branch and will update the pull request and rerun the tests. Adding new commits to a branch will generally work without complications.

@jkhillier

Copy link
Copy Markdown
Contributor Author

OK - thank you. One more thing on how GitHub works added to my notes :-)

@wkearn wkearn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, @jkhillier. I think wavInterpCombii/wavInterpCombii_inner is ready to go. On to the next functions, which, looking at wavelet, are wavInterpComb, PostProcess and Interpolate.

FYI: Not entirely why - in detail - I'm refactoring, but happy to keep going.

libtopotoolbox has fairly strict rules about what its functions should look like in order to interface smoothly with higher-level languages. Some of these rules are

  1. We don't use malloc or related dynamic memory allocation functions.
  2. We don't do any input/output through the operating system
  3. All data is passed in by reference (as pointers to pre-allocated arrays).

On the other hand, swt.c has been written as a standalone program, so it has to allocate and do I/O. The hard part is converting swt.c from its previous style to one that will work in libtopotoolbox without changing its behavior. You could rewrite swt.c in the libtopotoolbox style from scratch, but that brings a risk that you leave out some important element that changes the behavior, and we don't notice until later. Working incrementally, leaving as much of the overall structure in place, extracting the core computational routines and testing along the way makes sure that bugs don't creep in.

At the end of all of this, it is the core functions like wavInterpCombii_inner that we'll consider adding to libtopotoolbox. We will reimplement functions like wavInterpCombii, which extract necessary data, call the core function, and then package the results into a usable form, in Python, MATLAB and R. Separating them out also helps us see more clearly what we need to implement in the higher-level languages and how we need to call the core routines.

There are other benefits of the incremental refactoring approach as well. I learn how SWT works by going through the changes that you are making one at a time. You learn the git-based workflow that we will use to maintain and update SWT in the future.

@wkearn
wkearn merged commit 33ec447 into TopoToolbox:main Jul 28, 2026
2 checks passed
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