Skip to content

7 jh refactor - #20

Open
jkhillier wants to merge 5 commits into
TopoToolbox:mainfrom
jkhillier:7_JH_refactor
Open

7 jh refactor#20
jkhillier wants to merge 5 commits into
TopoToolbox:mainfrom
jkhillier:7_JH_refactor

Conversation

@jkhillier

Copy link
Copy Markdown
Contributor

7_JH_refactor – Commit #1 - print and ptrdiff_t done in ReadData(). Commit #2 - wavInterpComb() print and ptrdiff_t done. Commit #3 – getlineJH() & Interpolate() changed to ptrdiff_t, PostProcess() removed prints. Commit #4 - PostProcess() Changed int to ptrdiff_t. No more changes needed to Interpolate. Commit #5 - quick_select() and tempInterp not used and removed.

• There are no more functions in wavelet that need checking out.
• fopen() is only in ReadData() - passes file pointer to getlineJH(), in wavInterpCombii() – passes pointer to getlineJH(). In wavelet() and
• fprintf() is in wavelet() only, which opens and closes 6 files.
• getlineJH() and ReadData(); these only read data – so I’m guessing they don’t need an ‘inner’ function. wavInterpCombii() – changed to have an inner function.

This only leaves wavelet() I think ….. how do you suggest I re-factor this, since it’s mainly single commands and fprintf()?

@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.

These changes look good, @jkhillier. There is one remaining unused variable warning that needs to be fixed:

src/swt.c: In function ‘wavInterpComb’:
src/swt.c:1282:9: error: variable ‘ratio’ set but not used [-Werror=unused-but-set-variable]
 1282 |   float ratio;        /*ratio: outline length / Area */
      |         ^~~~~

The ratio variable is set on line 1418, but not used because you commented out the print statement that uses it. If you remove line 1418, you'll get rid of this warning, but you'll now get a warning that float ratio is unused, so you'll need to delete its declaration on line 1282.

I think the automated checks may not have run properly when you submitted the PR, but I have rerun them again, and this seems like the only error you need to resolve.

@wkearn

wkearn commented Aug 10, 2026

Copy link
Copy Markdown
Member

This only leaves wavelet() I think ….. how do you suggest I re-factor this, since it’s mainly single commands and fprintf()?

At first, I would have said that we should leave wavelet as it is. We would then implement wavelet in Python/MATLAB and call libtopotoolbox versions of wavCompute, wavInterpii etc.

However, we currently save the wavelet object coefficients to a file (fwavObjCoeff), which we then reopen and read from in wavInterpCombii. Ideally, we would pass these data as arrays into wavInterpCombii. Fortunately we already have a version of wavInterpCombii that accepts data as arrays: your inner function wavInterpCombii_inner!

So I think the next step is to change wavelet so that it calls wavInterpCombii_inner directly rather than wavInterpCombii. I think this requires defining Arr1, Arr2, and Arr3 as static arrays (i.e. float Arr1[MAXCOLUMNS]; perhaps give them more informative names) in wavelet and then filling them up where you save the coefficients to file in this loop:

SWT/src/swt.c

Lines 975 to 981 in 33ec447

for(j = 0; j < numberlines; j++)
{
if (WavCoeff[j] > 0)
{
fprintf(fwavObjCoeff, "%d %f %f %f\n", j, scale, WavCoeff[j], distance[j]);
}
}

You'll still need to save the coefficients to file because we do check that file in the tests.

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