Skip to content

Conversation

@cmaureir
Copy link
Collaborator

Using 50 is really low, and in modern times we do not
need to care much, then using 200 is totally fine.

Issue-number: 4

Signed-off-by: Cristian Maureira-Fredes cmaureirafredes@gmail.com

Using 50 is really low, and in modern times we do not
need to care much, then using 200 is totally fine.

Issue-number: 4

Signed-off-by: Cristian Maureira-Fredes <cmaureirafredes@gmail.com>
@teuben
Copy link
Contributor

teuben commented Mar 28, 2018

Changing 50 to 200 is NOT the best solution:

  1. at least use a #define for that lenght, and use that throughout
  2. closer examination of the code should tell you how to use malloc() or something like that to use the correct amount.

I have a repo copy in https://github.com/teuben/mcluster which uses the following technique:

//Open output files
char *PARfile, *NBODYfile, *SSEfile;
FILE *PAR, *NBODY, *SSE12;
PARfile = strcat(output,".input");
PAR = fopen(PARfile,"w");
NBODYfile = strcat(output,".fort.10");
NBODY = fopen(NBODYfile,"w");

look at the start of main.c

This type of code is repeated over and over again, and could easily be uplifted in some utiity code so this doesn't have to be retyped all over the code. There was another issue on code-restructuring, which might involve this as well.

I thought I had made a pull request from my version, but seems not. Needs to be done or discussed what do to.

@cmaureir
Copy link
Collaborator Author

Well, I want it to be minimalistic as possible, this piece of code can be optimised in many ways.
I have rewritten the whole code to separate functions, creating structures, and many others things, but I cannot open a pull request that changes everything, that is why I monkey-patched the small open issues, and I was trying to submit changes little by little.

I think C is no the best way of doing things, so I have also a set of patched that move the whole thing to C++, not using classes, because that will scare people, but just using all the perks of C++ standard library.

So yeah, sorry for just increase the size of the buffers, but big changes are usually slow to get merge :)

@cmaureir
Copy link
Collaborator Author

This is the branch that I currently have with the whole restructuration of the project https://github.com/cmaureir/mcluster/tree/improvement

  • I'm moving everything to headers and lib files
  • After that I will check the proper functionallity of all the versions
  • To start, I added to big structures to hold the information of each particle and all the options that one can specify from the command line, but a deep look is necessary to have the proper structures, since there are many variables that don't really need to be inside them.
  • When this is working, you can clearly see that most of the functions are repeating code, so there is a real need to improve that, since I'm confident the code can be shorter as it's now.
  • The last stage will be to integrate std library algorithms to avoid the large amount of loops tha algorithms that the code currently have.

I'm more than happy to hear any further concern regarding the code, or maybe another important aspect that I'm missing.

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