Skip to content
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

Adaptive modification #10

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Adaptive modification #10

wants to merge 1 commit into from

Conversation

urdit
Copy link

@urdit urdit commented Sep 2, 2024

To read and merge data stored in .uff files, I have made some modification.

  1. function: read_and_process_large_file
  2. function: saveFRFtoMat etc.

To read and merge data stored in .uff files, I have made some modification.
1. function: read_and_process_large_file
2. function: saveFRFtoMat
etc.
@urdit urdit closed this Sep 2, 2024
@urdit urdit reopened this Sep 2, 2024
@urdit
Copy link
Author

urdit commented Sep 2, 2024

Hello, I made some modifications to the uff file and solved some error problems.

@awiawi
Copy link
Contributor

awiawi commented Sep 3, 2024

Hello, thanks for extending this repo! 👍

Unfortunately, neither the person who wrote this code nor I are still working on this topic anymore, so I will probably not be able to give quality any feedback. Moreover, I do not have Matlab installed anymore, so I need to see if I find a machine where I can test your code.

A few requests from my side:

  • Could you please translate the Chinese comments to English? This would help me understand the intent of the code.
  • Could you please give a high level explanation of each file (I think at best at the top of the file), to give a quick overview over what is happening in each file?
  • You copied the main script modalAnalysis_pLSCF_main.m and only changed some parameters in it. I think we should remove this file, as it could confuse the user which one to use. If these parameters are necessary for your examples, then we should maybe think about moving the settings to a separate file.
  • Some names of files you added do not provide meaningful information about the content (have2try.m, it also seems not to be used). Could you please make sure, that you only commit necessary files with meaningful names? I think maybe some other files (.uff) which are loaded in one script are missing, so I guess I will not be able to run this code.
  • Could you please explain in the Pull Request a little bit about what your extension does, e.g. what was not possible before that is now possible?

Moreover, from skimming through your code, I think you introduced multiple changes (reading uff files and what is done in double.m). If these are independent additions to the code, I would invite you to open separate Pull Requests for each change introduced to make reviewing and discussing easier.

Finally, I found some existing implementations for reading uff files in a quick search on Matlab file exchange:

At a first glance, the license seems to be compatible for including these files here. They are far more complex then the function you wrote, do you think it makes more sense to use your approach or include any of the already existing (and hopefully well tested) files for reading uff files?

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